Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop support for verification of detached cleartext signatures #1265

Merged
merged 5 commits into from Mar 18, 2021

Conversation

larabr
Copy link
Collaborator

@larabr larabr commented Mar 12, 2021

When verifying cleartext messages, it is no longer possible to pass the signature separately -- this is because a cleartext message in itself should include the signature.
In the past, we allowed to detach-sign a cleartext message, so we have kept supporting verifying detached cleartext signatures up until now. However, the concept of "detached cleartext signature" does not make sense.

In practice, given a signed cleartext message:

const cleartextMessage = await openpgp.sign({
    message: await openpgp.createCleartextMessage({ text: 'test      ' }),
    signingKeys
});

it should be verified as:

 const verified = await openpgp.verify({
      message: await openpgp.readCleartextMessage({ cleartextMessage }),
      verificationKeys
});

With this PR, the following is no longer supported:

const verified = await openpgp.verify({
    message: await openpgp.createCleartextMessage({ text: 'test      ' }),
    verificationKeys,
    // cannot pass signature with cleartext message
    signature: (await openpgp.readCleartextMessage({ cleartextMessage })).signature
})

if you have a detached signature over some cleartext data (generated using previous versions of OpenPGP.js), you can still verify it by passing a Message generated from the text data, with trailing spaces removed:

const verified = await openpgp.verify({
      message: await openpgp.createMessage({ text: removeTrailingSpaces('test      ') }),
      verificationKeys,
      signature: (await openpgp.readCleartextMessage({ cleartextMessage })).signature
})

(with removeTrailingSpaces copied from here, for example).


Update from the future: detached cleartext signatures containing trailing whitespace followed by \r\n line endings are affected by the normalization issue fixed in #1548, as openpgp.CleartextMessage.fromText('...') would incorrectly normalize some data since v3.0.9. To verify a detached text signature generated prior to OpenPGP.js v3.0.9, use the version of removeTrailingSpaces from here, instead.


Another update from the future: when verifying binary signatures over text data (generated by other OpenPGP implementations - OpenPGP.js always generates text signatures over text data), the above is still not quite complete. Creating a cleartext message object would also normalize the newlines to \r\n. Hence, if there's a chance that the text was changed from \r\n to \n newlines during transport (after signing but before verification), the newlines need to be normalized back to \r\n manually. Here's a full example using OpenPGP.js v5 to verify such a signature:

function removeTrailingSpaces() {
    return text.split('\n').map(line => {
        let i = line.length - 1;
        for (; i >= 0 && (line[i] === ' ' || line[i] === '\t'); i--);
        return line.substr(0, i + 1);
    }).join('\n');
}

const message = 'some message to verify...';
const text = removeTrailingSpaces(message.replaceAll('\r\n', '\n').replaceAll('\r', '\n')).replaceAll('\n', '\r\n');
const verified = await openpgp.verify({
      message: await openpgp.createMessage({ text }),
      verificationKeys,
      signature: await openpgp.readSignature({ armoredSignature: ... }) // or binarySignature: ...
});

@tomholub
Copy link
Contributor

Hello - is there an issue associated that clarifies this change? This seems like something we have a usecase for, but I may have missed something.

@larabr
Copy link
Collaborator Author

larabr commented Mar 12, 2021

Hello - is there an issue associated that clarifies this change? This seems like something we have a usecase for, but I may have missed something.

I'll add details later

@twiss
Copy link
Member

twiss commented Mar 12, 2021

For additional context, in OpenPGP.js v3, if you did:

await openpgp.sign({
    data: 'test      ',
    privateKeys,
    detached: true
});

then it would internally construct a cleartext message, and detached-sign it. Logically speaking, this doesn't make much sense, since a detached cleartext signature doesn't exist, it's just a detached signature. Practically speaking, it doesn't much matter; the only difference between a cleartext signature and a normal signature is that for the former, we strip trailing spaces from each line (the spec requires this for cleartext signatures, but not for detached signatures - or text signatures in general).

In OpenPGP.js v4, we switched to passing messages instead of data (this was necessary for streaming support, since when passing a stream, it's not obvious whether it contains text or binary data, hence the message.fromText/fromBinary), so then you instead had to do:

await openpgp.sign({
    message: openpgp.cleartext.fromText('test      '),
    privateKeys,
    detached: true
});

(if you wanted to keep the same behavior of removing trailing spaces - otherwise, openpgp.message.fromText would also work).

In OpenPGP.js v5, to keep the same behavior, you'll have to do:

await openpgp.sign({
    message: openpgp.Message.fromText(removeTrailingSpaces('test      ')),
    privateKeys,
    detached: true
});

(with removeTrailingSpaces copied from here, for example).

This is more verbose, but at least now it's obvious what's going on. In principle, we'd prefer people just use openpgp.Message.fromText directly - there's no particular reason to remove trailing spaces, except if you need to be backwards compatible with this quirk from OpenPGP.js v3.

The above change has already been done in the v5 branch, this PR does the same for openpgp.verify.

@larabr larabr marked this pull request as ready for review March 16, 2021 10:31
@twiss twiss merged commit 3e808c1 into openpgpjs:master Mar 18, 2021
@larabr larabr deleted the drop-detach-verify branch June 10, 2021 09:35
@twiss
Copy link
Member

twiss commented Sep 2, 2022

For posterity: the original example code was unfortunately not quite complete in all cases. I've updated the PR description above with code examples and an explanation of the cases in which it's needed.

For additional details, read on:

Creating a cleartext message also normalized the newlines to \r\n. Verifying a text signature does so as well (also on a normal message), but verifying a binary signature does not. For signatures created by OpenPGP.js, this doesn't matter, as OpenPGP.js always creates text signatures for text messages. However, when verifying signatures from other implementations, if a binary signature was created over text with \r\n newlines, and the text was then modified to use \n newlines (during transport, for example), and the signature would then get verified by OpenPGP.js on the modified text, this would work in v4 but break with the original example code above.

Unfortunately, the behavior prior to v3.0.9 was again different. Prior to that version, OpenPGP.js would also remove trailing spaces in lines with \r\n newlines, but from v3.0.9 to v5.3.1 (inclusive), only trailing spaces before \n were removed, due to a bug which was fixed in #1548. (In v5, however, this is only relevant for signing, as verifying detached signatures on cleartext messages is no longer supported, as per this PR :))

All of this is mostly only relevant for PGP/MIME signatures, which should have trailing spaces stripped and newlines normalized to \r\n. There, the behavior prior to v3.0.9 is thus correct. So, applications verifying PGP/MIME signatures should, in principle, have already had custom code to strip trailing spaces and normalize newlines, in which case all of the above wouldn't cause issues. But, it's possible that, in cases where the application only handled removing trailing spaces but not normalizing newlines, or normalized newlines to \n, the above would cause bugs. Don't ask me how I know :)

Apologies for the confusion!

(The reason we went to all this trouble, though, and the main advantage of having all this logic spelled out in the application, rather than hidden away in OpenPGP.js, is that this logic cleanly maps from the PGP/MIME specification, RFC 3156, which is separate from the OpenPGP specification, and having this logic in the application makes it easier to verify that the application matches the behavior required for its specific use-case.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants