Test private key parsing for commentless edge case #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpalmer
force-pushed
the
test-commentless-parsing
branch
from
45083f0
to
b415714
Compare
|
Based on this comment, I'll be reworking this PR to not make zero length reads. |
Adjust to the changes to `base64ct` that reject zero-length decode() calls as invalid. The underlying trigger for this change was to avoid problems with decoding certain OpenSSH private keys. If a "PEM-like" OpenSSH format private key: 1. has no comment; 2. is of a length such that there is no padding bytes at the end of the "Unencrypted list of N private keys"; ***and*** 3. is of a length such that the base64-encoded form of the key does not require any `=` bytes for padding; then `ssh_key::private::PrivateKey::from_openssh` failed to successfully parse the key. The root cause was `base64ct` (prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads, *only* when the base64-encoded buffer was empty. This only happened for reading empty comments, because comments are at the end of the key. It *wouldn't* happen if: 1. the key had a comment, because that wouldn't be a zero-length read; 2. there were padding bytes in the "Unencrypted list of N private keys", because those padding bytes would still be in the buffer, so the buffer wasn't empty; *or* 3. there were base64 `=` padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes). Debugging this was... an amusement. The most reliable way to demonstrate this was with an ECDSA P-256 key without a comment (easily generated with `ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>`), because they're *usually* (but not *always*) an appropriate length to trigger the bug. Amusingly (cough), the existing ECDSA P-256 test key in `ssh-key/tests/examples/id_ecdsa_p256` is one of the outliers, as its `d` is 33 bytes, rather than 32, which means that key (when stripped of its comment) *doesn't* trigger the bug.
mpalmer
force-pushed
the
test-commentless-parsing
branch
from
b415714
to
2c6c66a
Compare
tarcieri
approved these changes
Apr 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a "PEM-like" OpenSSH format private key:
=bytes for padding;then
ssh_key::private::PrivateKey::from_opensshfails to successfully parse the key.The root cause is
base64ct(prior to RustCrypto/formats#1387) incorrectly rejecting zero-length reads when the base64-encoded buffer was empty. This wouldn't happen if:=padding characters, because those would cause the decoder to believe a read would succeed (even though it wouldn't if the read was longer than zero bytes).Debugging this was... an amusement.
The most reliable way to demonstrate this is with an ECDSA P-256 key without a comment (easily generated with
ssh-keygen -t ecdsa -b 256 -C '' -N '' -f <file>), because they usually (but not always) are of an appropriate length to trigger the bug. Amusingly (cough), the existing ECDSA P-256 key inssh-key/tests/examples/id_ecdsa_p256is one of the outliers, as itsdis 33 bytes, rather than 32, which means that key (when stripped of its comment) doesn't trigger the bug.This PR is marked as "Draft" because while it adds a test case for the bug, which I believe is valuable, the actual fix is in
base64ct, which I've referenced as aCargo.tomlpatch.Thus in its current form, presumably, this PR is not suitable for merge.
It is possible to work around the bug in
base64ctby guarding reads withreader.remaining_len() > 0tests in various places, but to me that seems far uglier than just updatingbase64ctand depending on a fixed version.However, if you'd like to keep the
base64ctdependency open, I can rework this PR to include guards in the appropriate spot(s).