Discard invalid incrementalMacs

This commit is contained in:
trevor-signal
2025-09-08 16:19:17 -04:00
committed by GitHub
parent ebdf651dca
commit b92c0e95e8
14 changed files with 304 additions and 88 deletions

View File

@@ -134,7 +134,7 @@ async function safeDecryptToSink(
});
file.on('error', (error: Error) => {
log.warn(
'safeDecryptToSync/incremental: growing-file emitted an error:',
'safeDecryptToSink/incremental: growing-file emitted an error:',
Errors.toLogFormat(error)
);
});

View File

@@ -130,7 +130,7 @@
"@react-aria/utils": "3.25.3",
"@react-spring/web": "9.7.5",
"@react-types/shared": "3.27.0",
"@signalapp/libsignal-client": "0.79.1",
"@signalapp/libsignal-client": "0.80.0",
"@signalapp/minimask": "1.0.1",
"@signalapp/quill-cjs": "2.1.2",
"@signalapp/ringrtc": "2.57.1",

10
pnpm-lock.yaml generated
View File

@@ -126,8 +126,8 @@ importers:
specifier: 3.27.0
version: 3.27.0(react@18.3.1)
'@signalapp/libsignal-client':
specifier: 0.79.1
version: 0.79.1
specifier: 0.80.0
version: 0.80.0
'@signalapp/minimask':
specifier: 1.0.1
version: 1.0.1
@@ -3296,8 +3296,8 @@ packages:
'@signalapp/libsignal-client@0.76.7':
resolution: {integrity: sha512-iGWTlFkko7IKlm96Iy91Wz5sIN089nj02ifOk6BWtLzeVi0kFaNj+jK26Sl1JRXy/VfXevcYtiOivOg43BPqpg==}
'@signalapp/libsignal-client@0.79.1':
resolution: {integrity: sha512-mL+SCJEbDqLYpd5JtA53JgjuYinrWF7jSonA2bD3HJMdHyAJ1jP+ThPD3Vh71yR+EhTCflyr5Tm43sQ0KoLtlg==}
'@signalapp/libsignal-client@0.80.0':
resolution: {integrity: sha512-cOFORDUUSdQFjQ8RDA8niC1UIoFRi7+Dd8ZNizGcJ2Q16D9RhKU9yZ0/VIef+mWu/iJcPovoC3GCvw0nE881vw==}
'@signalapp/minimask@1.0.1':
resolution: {integrity: sha512-QAwo0joA60urTNbW9RIz6vLKQjy+jdVtH7cvY0wD9PVooD46MAjE40MLssp4xUJrph91n2XvtJ3pbEUDrmT2AA==}
@@ -13943,7 +13943,7 @@ snapshots:
type-fest: 4.26.1
uuid: 11.0.2
'@signalapp/libsignal-client@0.79.1':
'@signalapp/libsignal-client@0.80.0':
dependencies:
node-gyp-build: 4.8.4
type-fest: 4.26.1

View File

@@ -582,9 +582,10 @@ export async function decryptAndReencryptLocally(
};
} catch (error) {
log.error(
`${logId}: Failed to decrypt attachment`,
`${logId}: Failed to decrypt and reencrypt attachment`,
Errors.toLogFormat(error)
);
await safeUnlink(absoluteTargetPath);
throw error;
} finally {

View File

@@ -13,7 +13,10 @@ import {
AttachmentDownloadUrgency,
coreAttachmentDownloadJobSchema,
} from '../types/AttachmentDownload';
import { downloadAttachment as downloadAttachmentUtil } from '../util/downloadAttachment';
import {
downloadAttachment as downloadAttachmentUtil,
isIncrementalMacVerificationError,
} from '../util/downloadAttachment';
import { DataReader, DataWriter } from '../sql/Client';
import { getValue } from '../RemoteConfig';
@@ -27,6 +30,7 @@ import {
canAttachmentHaveThumbnail,
shouldAttachmentEndUpInRemoteBackup,
getUndownloadedAttachmentSignature,
isIncremental,
} from '../types/Attachment';
import { type ReadonlyMessageAttributesType } from '../model-types.d';
import { getMessageById } from '../messages/getMessageById';
@@ -44,7 +48,7 @@ import {
type JobManagerJobResultType,
type JobManagerJobType,
} from './JobManager';
import { IMAGE_JPEG } from '../types/MIME';
import { IMAGE_WEBP } from '../types/MIME';
import { AttachmentDownloadSource } from '../sql/Interface';
import { drop } from '../util/drop';
import {
@@ -506,8 +510,8 @@ async function runDownloadAttachmentJob({
if (result.downloadedVariant === AttachmentVariant.ThumbnailFromBackup) {
return {
status: 'finished',
newJob: { ...job, attachment: result.attachmentWithThumbnail },
status: 'retry',
updatedJob: { ...job, attachment: result.attachmentWithThumbnail },
};
}
@@ -543,6 +547,38 @@ async function runDownloadAttachmentJob({
return { status: 'finished' };
}
if (isIncrementalMacVerificationError(error)) {
log.warn(
'Attachment decryption failed with incrementalMac verification error; dropping incrementalMac'
);
// If incrementalMac fails verification, we will delete it and retry (without
// streaming support)
strictAssert(isIncremental(job.attachment), 'must have incrementalMac');
const attachmentWithoutIncrementalMac: AttachmentType = {
...job.attachment,
pending: false,
incrementalMac: undefined,
chunkSize: undefined,
};
// Double-check it no longer supports incremental playback just to make sure we
// avoid any loops
strictAssert(
!isIncremental(attachmentWithoutIncrementalMac),
'no longer has incrementalMac'
);
await addAttachmentToMessage(
message.id,
attachmentWithoutIncrementalMac,
logId,
{ type: job.attachmentType }
);
return {
status: 'retry',
updatedJob: { ...job, attachment: attachmentWithoutIncrementalMac },
};
}
if (error instanceof AttachmentPermanentlyUndownloadableError) {
const canBackfill =
job.isManualDownload &&
@@ -672,27 +708,28 @@ export async function runDownloadAttachmentJobInner({
canAttachmentHaveThumbnail(attachment) &&
!wasAttachmentImportedFromLocalBackup;
const preferBackupThumbnail =
const attemptBackupThumbnailFirst =
isForCurrentlyVisibleMessage && mightHaveBackupThumbnailToDownload;
if (preferBackupThumbnail) {
logId += '.preferringBackupThumbnail';
}
let attachmentWithBackupThumbnail: AttachmentType | undefined;
if (attemptBackupThumbnailFirst) {
logId += '.preferringBackupThumbnail';
if (preferBackupThumbnail) {
try {
const attachmentWithThumbnail = await downloadBackupThumbnail({
attachmentWithBackupThumbnail = await downloadBackupThumbnail({
attachment,
abortSignal,
dependencies,
});
await addAttachmentToMessage(messageId, attachmentWithThumbnail, logId, {
type: attachmentType,
});
return {
downloadedVariant: AttachmentVariant.ThumbnailFromBackup,
attachmentWithThumbnail,
};
await addAttachmentToMessage(
messageId,
attachmentWithBackupThumbnail,
logId,
{
type: attachmentType,
}
);
} catch (e) {
log.warn(
`${logId}: error when trying to download thumbnail`,
@@ -801,32 +838,25 @@ export async function runDownloadAttachmentJobInner({
);
return { downloadedVariant: AttachmentVariant.Default };
} catch (error) {
if (mightHaveBackupThumbnailToDownload && !preferBackupThumbnail) {
if (mightHaveBackupThumbnailToDownload && !attemptBackupThumbnailFirst) {
log.error(
`${logId}: failed to download fullsize attachment, falling back to backup thumbnail`,
Errors.toLogFormat(error)
);
try {
const attachmentWithThumbnail = omit(
await downloadBackupThumbnail({
attachment,
abortSignal,
dependencies,
}),
'pending'
);
attachmentWithBackupThumbnail = await downloadBackupThumbnail({
attachment,
abortSignal,
dependencies,
});
await addAttachmentToMessage(
messageId,
attachmentWithThumbnail,
{ ...attachment, pending: false },
logId,
{
type: attachmentType,
}
);
return {
downloadedVariant: AttachmentVariant.ThumbnailFromBackup,
attachmentWithThumbnail,
};
} catch (thumbnailError) {
log.error(
`${logId}: fallback attempt to download thumbnail failed`,
@@ -835,6 +865,13 @@ export async function runDownloadAttachmentJobInner({
}
}
if (attachmentWithBackupThumbnail) {
return {
downloadedVariant: AttachmentVariant.ThumbnailFromBackup,
attachmentWithThumbnail: attachmentWithBackupThumbnail,
};
}
let showToast = false;
// Show toast if manual download failed
@@ -889,7 +926,7 @@ async function downloadBackupThumbnail({
const attachmentWithThumbnail = {
...attachment,
thumbnailFromBackup: {
contentType: IMAGE_JPEG,
contentType: IMAGE_WEBP,
...downloadedThumbnail,
size: calculatedSize,
},

View File

@@ -66,10 +66,8 @@ export type JobManagerParamsType<
const DEFAULT_TICK_INTERVAL = MINUTE;
export type JobManagerJobResultType<CoreJobType> =
| {
status: 'retry';
}
| { status: 'finished'; newJob?: CoreJobType }
| { status: 'retry'; updatedJob?: CoreJobType & JobManagerJobType }
| { status: 'finished' }
| { status: 'rate-limited'; pauseDurationMs: number };
export type ActiveJobData<CoreJobType> = {
@@ -287,6 +285,7 @@ export abstract class JobManager<CoreJobType> {
return;
}
const isFirstAttempt = job.attempts === 0;
const isLastAttempt =
job.attempts + 1 >=
(this.params.getRetryConfig(job).maxAttempts ?? Infinity);
@@ -303,7 +302,9 @@ export abstract class JobManager<CoreJobType> {
this.#handleJobStartPromises(job);
jobRunResult = await runJobPromise;
const { status } = jobRunResult;
log.info(`${logId}: job completed with status: ${status}`);
log.info(
`${logId}: job completed with status: ${status}${status === 'retry' && jobRunResult.updatedJob ? ' with updated job' : ''}`
);
switch (status) {
case 'finished':
@@ -313,7 +314,13 @@ export abstract class JobManager<CoreJobType> {
if (isLastAttempt) {
throw new Error('Cannot retry on last attempt');
}
await this.#retryJobLater(job);
// If we get an updated job, retry it without delay only if it's the first
// attempt (to avoid loops)
if (isFirstAttempt && jobRunResult.updatedJob) {
await this.#retryJobWithoutDelay(jobRunResult.updatedJob);
} else {
await this.#retryJobLater(jobRunResult.updatedJob ?? job);
}
return;
case 'rate-limited':
log.info(
@@ -334,18 +341,21 @@ export abstract class JobManager<CoreJobType> {
}
} finally {
this.#removeRunningJob(job);
if (jobRunResult?.status === 'finished') {
if (jobRunResult.newJob) {
log.info(
`${logId}: adding new job as a result of this one completing`
);
await this.addJob(jobRunResult.newJob);
}
}
drop(this.maybeStartJobs());
}
}
async #retryJobWithoutDelay(job: CoreJobType & JobManagerJobType) {
const now = Date.now();
await this.params.saveJob({
...job,
active: false,
attempts: job.attempts + 1,
retryAfter: now,
lastAttemptTimestamp: now,
});
}
async #retryJobLater(job: CoreJobType & JobManagerJobType) {
const now = Date.now();
await this.params.saveJob({

View File

@@ -856,7 +856,7 @@ describe('Crypto', () => {
plaintextHash,
modifyIncrementalMac: true,
}),
/Corrupted/
/^Corrupted input data/
);
} finally {
unlinkSync(sourcePath);

View File

@@ -527,6 +527,44 @@ describe('AttachmentDownloadManager/JobManager', () => {
await jobAttempts[1].completed;
});
it('retries job with updated job if provided', async () => {
strictAssert(downloadManager, 'must exist');
const job = (
await addJobs(1, {
source: AttachmentDownloadSource.BACKUP_IMPORT,
})
)[0];
const jobAttempts = getPromisesForAttempts(job, 3);
runJob.callsFake(async args => {
return new Promise(resolve => {
Promise.resolve().then(() => {
resolve({
status: 'retry',
updatedJob: {
...args.job,
attachment: { ...job.attachment, caption: 'retried' },
},
});
});
});
});
await downloadManager?.start();
await jobAttempts[0].completed;
assertRunJobCalledWith([job]);
await jobAttempts[1].completed;
assert.deepStrictEqual(
runJob.getCall(0).args[0].job.attachment,
job.attachment
);
assert.deepStrictEqual(runJob.getCall(1).args[0].job.attachment, {
...job.attachment,
caption: 'retried',
});
});
describe('will drop jobs from non-media backup imports that are old', () => {
it('will not queue attachments older than 90 days (2 * message queue time)', async () => {
hasMediaBackups.returns(false);
@@ -660,12 +698,20 @@ describe('AttachmentDownloadManager/runDownloadAttachmentJob', () => {
AttachmentVariant.Default
);
});
it('will download thumbnail if attachment is from backup', async () => {
it('will download thumbnail first if attachment is from backup', async () => {
const job = composeJob({
messageId: '1',
receivedAt: 1,
});
downloadAttachment = sandbox.stub().callsFake(({ options }) => {
if (options.variant === AttachmentVariant.ThumbnailFromBackup) {
return Promise.resolve(downloadedAttachment);
}
throw new Error('error while downloading');
});
const result = await runDownloadAttachmentJobInner({
job,
isForCurrentlyVisibleMessage: true,
@@ -692,16 +738,23 @@ describe('AttachmentDownloadManager/runDownloadAttachmentJob', () => {
result.attachmentWithThumbnail.thumbnailFromBackup?.path,
'/path/to/file'
);
assert.strictEqual(downloadAttachment.callCount, 1);
assert.strictEqual(downloadAttachment.callCount, 2);
const downloadCallArgs = downloadAttachment.getCall(0).args[0];
assert.deepStrictEqual(downloadCallArgs.attachment, job.attachment);
const firstDownloadCallArgs = downloadAttachment.getCall(0).args[0];
assert.deepStrictEqual(firstDownloadCallArgs.attachment, job.attachment);
assert.deepStrictEqual(
downloadCallArgs.options.variant,
firstDownloadCallArgs.options.variant,
AttachmentVariant.ThumbnailFromBackup
);
const secondDownloadCallArgs = downloadAttachment.getCall(1).args[0];
assert.deepStrictEqual(
secondDownloadCallArgs.options.variant,
AttachmentVariant.Default
);
});
it('will download full size if thumbnail already backed up', async () => {
it('will download full size if backup thumbnail already downloaded', async () => {
const job = composeJob({
messageId: '1',
receivedAt: 1,
@@ -739,7 +792,10 @@ describe('AttachmentDownloadManager/runDownloadAttachmentJob', () => {
});
it('will attempt to download full size if thumbnail fails', async () => {
downloadAttachment = sandbox.stub().callsFake(() => {
downloadAttachment = sandbox.stub().callsFake(({ options }) => {
if (options.variant === AttachmentVariant.Default) {
return Promise.resolve(downloadedAttachment);
}
throw new Error('error while downloading');
});
@@ -748,22 +804,20 @@ describe('AttachmentDownloadManager/runDownloadAttachmentJob', () => {
receivedAt: 1,
});
await assert.isRejected(
runDownloadAttachmentJobInner({
job,
isForCurrentlyVisibleMessage: true,
hasMediaBackups: true,
abortSignal: abortController.signal,
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteDownloadData,
downloadAttachment,
processNewAttachment,
},
})
);
const result = await runDownloadAttachmentJobInner({
job,
isForCurrentlyVisibleMessage: true,
hasMediaBackups: true,
abortSignal: abortController.signal,
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteDownloadData,
downloadAttachment,
processNewAttachment,
},
});
assert.strictEqual(result.downloadedVariant, AttachmentVariant.Default);
assert.strictEqual(downloadAttachment.callCount, 2);
const downloadCallArgs0 = downloadAttachment.getCall(0).args[0];

View File

@@ -219,7 +219,7 @@ describe('backups', function (this: Mocha.Suite) {
const catTimestamp = bootstrap.getTimestamp();
const plaintextCat = await readFile(CAT_PATH);
const ciphertextCat = await bootstrap.storeAttachmentOnCDN(
const ciphertextCat = await bootstrap.encryptAndStoreAttachmentOnCDN(
plaintextCat,
IMAGE_JPEG
);

View File

@@ -674,7 +674,7 @@ export class Bootstrap {
return join(this.#storagePath, 'attachments.noindex', relativePath);
}
public async storeAttachmentOnCDN(
public async encryptAndStoreAttachmentOnCDN(
data: Buffer,
contentType: MIMEType
): Promise<Proto.IAttachmentPointer> {
@@ -684,13 +684,13 @@ export class Bootstrap {
const passthrough = new PassThrough();
const [{ digest }] = await Promise.all([
const [{ digest, chunkSize, incrementalMac }] = await Promise.all([
encryptAttachmentV2({
keys,
plaintext: {
data,
},
needIncrementalMac: false,
needIncrementalMac: true,
sink: passthrough,
}),
this.server.storeAttachmentOnCdn(cdnNumber, cdnKey, passthrough),
@@ -703,6 +703,8 @@ export class Bootstrap {
cdnNumber,
key: keys,
digest,
chunkSize,
incrementalMac,
};
}

View File

@@ -6,6 +6,8 @@ import { assert } from 'chai';
import { expect } from 'playwright/test';
import { type PrimaryDevice, StorageState } from '@signalapp/mock-server';
import * as path from 'path';
import { readFile } from 'fs/promises';
import type { App } from '../playwright';
import { Bootstrap } from '../bootstrap';
import {
@@ -16,6 +18,8 @@ import {
} from '../helpers';
import * as durations from '../../util/durations';
import { strictAssert } from '../../util/assert';
import { VIDEO_MP4 } from '../../types/MIME';
import { toBase64 } from '../../Bytes';
export const debug = createDebug('mock:test:attachments');
@@ -27,6 +31,14 @@ const CAT_PATH = path.join(
'fixtures',
'cat-screenshot.png'
);
const VIDEO_PATH = path.join(
__dirname,
'..',
'..',
'..',
'fixtures',
'ghost-kitty.mp4'
);
describe('attachments', function (this: Mocha.Suite) {
this.timeout(durations.MINUTE);
@@ -121,4 +133,83 @@ describe('attachments', function (this: Mocha.Suite) {
assert.strictEqual(incomingAttachment?.key, sentAttachment?.key);
assert.strictEqual(incomingAttachment?.digest, sentAttachment?.digest);
});
it('can download videos with incrementalMac and is resilient to bad incrementalMacs', async () => {
const { desktop } = bootstrap;
const page = await app.getWindow();
await page.getByTestId(pinned.device.aci).click();
const plaintextVideo = await readFile(VIDEO_PATH);
const videoPointer1 = await bootstrap.encryptAndStoreAttachmentOnCDN(
plaintextVideo,
VIDEO_MP4
);
const videoPointer2 = await bootstrap.encryptAndStoreAttachmentOnCDN(
plaintextVideo,
VIDEO_MP4
);
const incrementalTimestamp = Date.now();
const badIncrementalTimestamp = incrementalTimestamp + 1;
await sendTextMessage({
from: pinned,
to: desktop,
desktop,
text: 'video with good incrementalMac',
attachments: [videoPointer1],
timestamp: incrementalTimestamp,
});
await sendTextMessage({
from: pinned,
to: desktop,
desktop,
text: 'video with bad incrementalMac',
attachments: [
{ ...videoPointer2, chunkSize: (videoPointer2.chunkSize ?? 42) + 1 },
],
timestamp: badIncrementalTimestamp,
});
await expect(
getMessageInTimelineByTimestamp(page, incrementalTimestamp).locator(
'img.module-image__image'
)
).toBeVisible();
await expect(
getMessageInTimelineByTimestamp(page, badIncrementalTimestamp).locator(
'img.module-image__image'
)
).toBeVisible();
// goodIncrementalMac preserved
{
const messageInDB = (
await app.getMessagesBySentAt(incrementalTimestamp)
)[0];
strictAssert(messageInDB, 'message exists in DB');
const attachmentInDB = messageInDB.attachments?.[0];
strictAssert(videoPointer1.incrementalMac, 'must exist');
strictAssert(videoPointer1.chunkSize, 'must exist');
assert.strictEqual(
attachmentInDB?.incrementalMac,
toBase64(videoPointer1.incrementalMac)
);
assert.strictEqual(attachmentInDB?.chunkSize, videoPointer1.chunkSize);
}
// badIncrementalMac removed
{
const messageInDB = (
await app.getMessagesBySentAt(badIncrementalTimestamp)
)[0];
strictAssert(messageInDB, 'message exists in DB');
const attachmentInDB = messageInDB.attachments?.[0];
strictAssert(videoPointer2.incrementalMac, 'must exist');
strictAssert(videoPointer2.chunkSize, 'must exist');
assert.strictEqual(attachmentInDB?.incrementalMac, undefined);
assert.strictEqual(attachmentInDB?.chunkSize, undefined);
}
});
});

View File

@@ -51,19 +51,19 @@ describe('attachment backfill', function (this: Mocha.Suite) {
const { unknownContacts } = bootstrap;
[unknownContact] = unknownContacts;
textAttachment = await bootstrap.storeAttachmentOnCDN(
textAttachment = await bootstrap.encryptAndStoreAttachmentOnCDN(
Buffer.from('look at this pic, it is gorgeous!'),
LONG_MESSAGE
);
const plaintextCat = await readFile(CAT_PATH);
catAttachment = await bootstrap.storeAttachmentOnCDN(
catAttachment = await bootstrap.encryptAndStoreAttachmentOnCDN(
plaintextCat,
IMAGE_JPEG
);
const plaintextSnow = await readFile(SNOW_PATH);
snowAttachment = await bootstrap.storeAttachmentOnCDN(
snowAttachment = await bootstrap.encryptAndStoreAttachmentOnCDN(
plaintextSnow,
IMAGE_JPEG
);

View File

@@ -164,7 +164,10 @@ export async function downloadAttachment(
}
// Start over if we go over the size
if (downloadOffset >= size && absoluteDownloadPath) {
if (
downloadOffset >= getAttachmentCiphertextLength(size) &&
absoluteDownloadPath
) {
log.warn('went over, retrying');
await safeUnlink(absoluteDownloadPath);
downloadOffset = 0;
@@ -310,7 +313,7 @@ export async function downloadAttachment(
// backup thumbnails don't get trimmed, so we just calculate the size as the
// ciphertextSize, less IV and MAC
const calculatedSize = downloadSize - IV_LENGTH - MAC_LENGTH;
return decryptAndReencryptLocally({
return await decryptAndReencryptLocally({
type: 'backupThumbnail',
ciphertextPath: cipherTextAbsolutePath,
idForLogging: logId,

View File

@@ -1,6 +1,6 @@
// Copyright 2020 Signal Messenger, LLC
// SPDX-License-Identifier: AGPL-3.0-only
import { ErrorCode, LibSignalErrorBase } from '@signalapp/libsignal-client';
import {
type AttachmentType,
AttachmentVariant,
@@ -68,6 +68,9 @@ export async function downloadAttachment({
onSizeUpdate(attachment.size);
return result;
} catch (error) {
if (isIncrementalMacVerificationError(error)) {
throw error;
}
// We also just log this error instead of throwing, since we want to still try to
// find it on the backup then transit tiers.
log.error(
@@ -90,6 +93,10 @@ export async function downloadAttachment({
}
);
} catch (error) {
if (isIncrementalMacVerificationError(error)) {
throw error;
}
const shouldFallbackToTransitTier =
variant !== AttachmentVariant.ThumbnailFromBackup;
@@ -128,6 +135,10 @@ export async function downloadAttachment({
}
);
} catch (error) {
if (isIncrementalMacVerificationError(error)) {
throw error;
}
if (mightBeOnBackupTierInTheFuture) {
// We don't want to throw the AttachmentPermanentlyUndownloadableError because we
// may just need to wait for this attachment to end up on the backup tier
@@ -150,3 +161,10 @@ export async function downloadAttachment({
}
}
}
export function isIncrementalMacVerificationError(error: unknown): boolean {
return (
error instanceof LibSignalErrorBase &&
error.code === ErrorCode.IncrementalMacVerificationFailed
);
}