Skip to content

Commit

Permalink
#5868 Display warning for incorrect pgp checksum (#5883)
Browse files Browse the repository at this point in the history
* feat: display warning for incorrect pgp checksum

* fix: ui test

* fix: pr reviews
  • Loading branch information
ioanmo226 authored Dec 24, 2024
1 parent e7636ca commit fbb5a07
Show file tree
Hide file tree
Showing 11 changed files with 83 additions and 15 deletions.
3 changes: 2 additions & 1 deletion extension/chrome/elements/pgp_block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ export class PgpBlockView extends View {
if (data?.separateQuotedContentAndRenderText) {
this.quoteModule.separateQuotedContentAndRenderText(
data.separateQuotedContentAndRenderText.decryptedContent,
data.separateQuotedContentAndRenderText.isHtml
data.separateQuotedContentAndRenderText.isHtml,
data.separateQuotedContentAndRenderText.isChecksumInvalid
);
}
if (data?.setFrameColor) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Xss } from '../../../js/common/platform/xss.js';
export class PgpBlockViewQuoteModule {
public constructor(private view: PgpBlockView) {}

public separateQuotedContentAndRenderText = (decryptedContent: string, isHtml: boolean) => {
public separateQuotedContentAndRenderText = (decryptedContent: string, isHtml: boolean, isChecksumInvalid: boolean) => {
if (isHtml) {
const message = $('<div>').html(Xss.htmlSanitizeKeepBasicTags(decryptedContent)); // xss-sanitized
let htmlBlockQuoteExists = false;
Expand All @@ -32,10 +32,10 @@ export class PgpBlockViewQuoteModule {
message[0].removeChild(shouldBeQuoted[i]);
quotedHtml += shouldBeQuoted[i].outerHTML;
}
this.view.renderModule.renderContent(message.html(), false);
this.view.renderModule.renderContent(message.html(), false, isChecksumInvalid);
this.appendCollapsedQuotedContentButton(quotedHtml, true);
} else {
this.view.renderModule.renderContent(decryptedContent, false);
this.view.renderModule.renderContent(decryptedContent, false, isChecksumInvalid);
}
} else {
const lines = decryptedContent.split(/\r?\n/);
Expand All @@ -62,7 +62,7 @@ export class PgpBlockViewQuoteModule {
// only got quoted part, no real text -> show everything as real text, without quoting
lines.push(...linesQuotedPart.splice(0, linesQuotedPart.length));
}
this.view.renderModule.renderContent(Str.escapeTextAsRenderableHtml(lines.join('\n')), false);
this.view.renderModule.renderContent(Str.escapeTextAsRenderableHtml(lines.join('\n')), false, isChecksumInvalid);
if (linesQuotedPart.join('').trim()) {
this.appendCollapsedQuotedContentButton(linesQuotedPart.join('\n'));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,11 @@ export class PgpBlockViewRenderModule {
});
};

public renderContent = (htmlContent: string, isErr: boolean) => {
public renderContent = (htmlContent: string, isErr: boolean, isChecksumInvalid = false) => {
let contentWithLink = linkifyHtml(htmlContent);

if (isChecksumInvalid) {
contentWithLink = `<div class="pgp-invalid-checksum">${Lang.pgpBlock.invalidCheckSum}</div>${contentWithLink}}`;
}
// Temporary workaround for an issue where 'cryptup_reply' divs are not being hidden when replying to all
// messages from the FES. The root cause is that FES currently returns the body of
// password message replies as 'text/plain', which does not hide the divs as intended.
Expand Down
7 changes: 7 additions & 0 deletions extension/css/cryptup.css
Original file line number Diff line number Diff line change
Expand Up @@ -1592,6 +1592,13 @@ td {
align-items: center;
}

.pgp-invalid-checksum {
border-left: 3px solid #d18826;
color: #d18826;
margin: 10px 0;
padding-left: 5px;
}

.pgp_badge {
opacity: 1;
margin-bottom: 1em;
Expand Down
54 changes: 54 additions & 0 deletions extension/js/common/core/crypto/key.ts
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,60 @@ export class KeyUtil {
return keys.map(k => ({ id: k.id, emails: k.emails, armored: KeyUtil.armor(k), family: k.family }));
}

public static validateChecksum = (armoredText: string): boolean => {
const lines = armoredText.split('\n').map(l => l.trim());

// Filter out known non-data lines
const dataCandidates = lines.filter(line => line.length > 0 && !line.startsWith('-----') && !line.startsWith('Version:') && !line.startsWith('Comment:'));

// Find checksum line
const checksumIndex = dataCandidates.findIndex(line => line.startsWith('='));
if (checksumIndex === -1) return false;

const checksumLine = dataCandidates[checksumIndex].slice(1);
const dataLines = dataCandidates.slice(0, checksumIndex);

// Decode checksum
let providedBytes: string;
try {
providedBytes = atob(checksumLine);
} catch {
return false;
}
if (providedBytes.length !== 3) return false;

const providedCRC = (providedBytes.charCodeAt(0) << 16) | (providedBytes.charCodeAt(1) << 8) | providedBytes.charCodeAt(2);

// Attempt to decode all data lines (some may not be base64)
const decodedChunks: string[] = [];
for (const line of dataLines) {
try {
decodedChunks.push(atob(line));
} catch {
// Not a valid base64 line, skip it
}
}

if (decodedChunks.length === 0) return false;

const rawData = decodedChunks.join('');
const dataBytes = new Uint8Array([...rawData].map(c => c.charCodeAt(0)));

return KeyUtil.crc24(dataBytes) === providedCRC;
};

private static crc24 = (dataBytes: Uint8Array): number => {
let crc = 0xb704ce;
for (const dataByte of dataBytes) {
crc ^= dataByte << 16;
for (let j = 0; j < 8; j++) {
crc <<= 1;
if (crc & 0x1000000) crc ^= 0x1864cfb;
}
}
return crc & 0xffffff;
};

private static getSortValue(pubinfo: PubkeyInfo): number {
const expirationSortValue = typeof pubinfo.pubkey.expiration === 'undefined' ? Infinity : pubinfo.pubkey.expiration;
// sort non-revoked first, then non-expired
Expand Down
2 changes: 2 additions & 0 deletions extension/js/common/lang.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ export const Lang = {
cannotLocate: 'Could not locate this message.',
brokenLink: 'It seems it contains a broken link.',
pwdMsgAskSenderUsePubkey: 'This appears to be a password-protected message. Please ask the sender to encrypt messages for your Public Key instead.',
invalidCheckSum:
'Warning: Checksum mismatch detected.\nThis indicates the message may have been altered or corrupted during transmission.\nDecryption may still succeed, but verify the message source and integrity if possible.',
},
compose: {
abortSending: 'A message is currently being sent. Closing the compose window may abort sending the message.\nAbort sending?',
Expand Down
8 changes: 5 additions & 3 deletions extension/js/common/message-renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,8 @@ export class MessageRenderer {
sigResult: VerifyRes | undefined,
renderModule: RenderInterface,
retryVerification: (() => Promise<VerifyRes | undefined>) | undefined,
plainSubject: string | undefined
plainSubject: string | undefined,
isChecksumInvalid = false
): Promise<{ publicKeys?: string[] }> => {
if (isEncrypted) {
renderModule.renderEncryptionStatus('encrypted');
Expand Down Expand Up @@ -603,7 +604,7 @@ export class MessageRenderer {
);
}
decryptedContent = this.clipMessageIfLimitExceeds(decryptedContent);
renderModule.separateQuotedContentAndRenderText(decryptedContent, isHtml);
renderModule.separateQuotedContentAndRenderText(decryptedContent, isHtml, isChecksumInvalid);
await MessageRenderer.renderPgpSignatureCheckResult(renderModule, sigResult, Boolean(signerEmail), retryVerification);
if (renderableAttachments.length) {
renderModule.renderInnerAttachments(renderableAttachments, isEncrypted);
Expand Down Expand Up @@ -743,7 +744,8 @@ export class MessageRenderer {
result.signature,
renderModule,
this.getRetryVerification(signerEmail, verificationPubs => MessageRenderer.decryptFunctionToVerifyRes(() => decrypt(verificationPubs))),
plainSubject
plainSubject,
!KeyUtil.validateChecksum(encryptedData.toString())
);
} else if (result.error.type === DecryptErrTypes.format) {
if (fallbackToPlainText) {
Expand Down
2 changes: 1 addition & 1 deletion extension/js/common/render-interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export interface RenderInterface extends RenderInterfaceBase {
renderPassphraseNeeded(longids: string[]): void;
renderErr(errBoxContent: string, renderRawMsg: string | undefined, errMsg?: string): void;
renderInnerAttachments(attachments: TransferableAttachment[], isEncrypted: boolean): void;
separateQuotedContentAndRenderText(decryptedContent: string, isHtml: boolean): void;
separateQuotedContentAndRenderText(decryptedContent: string, isHtml: boolean, isChecksumInvalid: boolean): void;
renderVerificationInProgress(): void;
renderSignatureOffline(retry: () => void): void;
}
2 changes: 1 addition & 1 deletion extension/js/common/render-message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export type MessageInfo = {
export type RenderMessage = {
done?: true;
resizePgpBlockFrame?: true;
separateQuotedContentAndRenderText?: { decryptedContent: string; isHtml: boolean };
separateQuotedContentAndRenderText?: { decryptedContent: string; isHtml: boolean; isChecksumInvalid: boolean };
renderText?: string;
progressOperation?: { operationId: string; text: string; perc?: number; init?: boolean };
setFrameColor?: 'green' | 'gray' | 'red';
Expand Down
4 changes: 2 additions & 2 deletions extension/js/common/render-relay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ export class RenderRelay implements RenderInterface {
this.relay({ resizePgpBlockFrame: true });
};

public separateQuotedContentAndRenderText = (decryptedContent: string, isHtml: boolean) => {
this.relay({ separateQuotedContentAndRenderText: { decryptedContent, isHtml } });
public separateQuotedContentAndRenderText = (decryptedContent: string, isHtml: boolean, isChecksumInvalid: boolean) => {
this.relay({ separateQuotedContentAndRenderText: { decryptedContent, isHtml, isChecksumInvalid } });
};

public renderText = (text: string) => {
Expand Down
2 changes: 1 addition & 1 deletion test/source/tests/decrypt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1882,7 +1882,7 @@ XZ8r4OC6sguP/yozWlkG+7dDxsgKQVBENeG6Lw==
browser,
threadId,
{
content: [expectedContent],
content: [expectedContent, 'Warning: Checksum mismatch detected'],
encryption: 'encrypted',
},
authHdr
Expand Down

0 comments on commit fbb5a07

Please sign in to comment.