Don't fail with InvalidLength when reading nothing at end of data #1387
Conversation
This file contains hidden or 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
added a commit
to mpalmer/rust-crypto-ssh
that referenced
this pull request
Apr 22, 2024
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` fails 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: 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 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 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
b64ct-decode-empty-at-end
branch
from
5222491 to
bc3c2fc
Compare
mpalmer
added a commit
to mpalmer/rust-crypto-ssh
that referenced
this pull request
Apr 22, 2024
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` fails 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: 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 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 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.
baloo
reviewed
Apr 22, 2024
Member
|
Is this really necessary and the best solution? Wouldn’t it be better not to perform a zero length read in the first place? |
Contributor
Author
If "don't perform zero length reads" is the best solution, that's cool, although in that case it would be better, IMO, if this method always rejected zero length reads. The thing that made this bug so hard to track down was the fact that the zero length read only failed in a very narrow set of circumstances. |
mpalmer
commented
Apr 22, 2024
Member
|
I think it's fine to always reject zero length reads |
Contributor
Author
|
Righto, I'll rework the PR to go that way. |
mpalmer
force-pushed
the
b64ct-decode-empty-at-end
branch
from
ad32016 to
ade0ecb
Compare
mpalmer
added a commit
to mpalmer/rust-crypto-ssh
that referenced
this pull request
Apr 24, 2024
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
b64ct-decode-empty-at-end
branch
from
ade0ecb to
b44f92d
Compare
mpalmer
force-pushed
the
b64ct-decode-empty-at-end
branch
from
b44f92d to
e72445c
Compare
Contributor
Author
|
OK, base64ct now errors on zero-length decode requests, with |
tarcieri
pushed a commit
to RustCrypto/SSH
that referenced
this pull request
Apr 24, 2024
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.
tarcieri
approved these changes
Apr 24, 2024
baloo
added a commit
to baloo/formats
that referenced
this pull request
Feb 26, 2025
Added - derive additional traits on alphabets (RustCrypto#1578) Changed - MSRV 1.85 // Edition 2024 (RustCrypto#1670) - reject zero-length decode requests (RustCrypto#1387) - use `core::error::Error` (RustCrypto#1681)
Merged
baloo
added a commit
that referenced
this pull request
Feb 26, 2025
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with [RustCrypto#1387]: RustCrypto#1387
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with an sha256WithRSAEncryption AlgorithmIdentifier where the parameters are null. [RustCrypto#1387]: RustCrypto#1387
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with [RustCrypto#1387]: RustCrypto#1387
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with an sha256WithRSAEncryption AlgorithmIdentifier where the parameters are null.
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with an sha256WithRSAEncryption AlgorithmIdentifier where the parameters are null.
tarcieri
added a commit
that referenced
this pull request
Mar 13, 2025
Member
|
Ended up reverting this as it's a breaking change: #1714 |
tarcieri
added a commit
that referenced
this pull request
Mar 13, 2025
Merged
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with an sha256WithRSAEncryption AlgorithmIdentifier where the parameters are null.
baloo
added a commit
to baloo/formats
that referenced
this pull request
Mar 13, 2025
Since base64ct 1.7.0 ([RustCrypto#1387]), it now rejects zero lengths reads. This happens to trigger with an sha256WithRSAEncryption AlgorithmIdentifier where the parameters are null.
tarcieri
pushed a commit
that referenced
this pull request
Mar 13, 2025
… of data (#1716) * der: pem reader rejects zero lengths reads [tests] Since base64ct 1.7.0 ([#1387]), it now rejects zero lengths reads. This happens to trigger with an sha256WithRSAEncryption AlgorithmIdentifier where the parameters are null. * base64ct: Don't fail with `InvalidLength` when reading nothing at end of data
scv35
pushed a commit
to scv35/SSH
that referenced
this pull request
Jul 4, 2025
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is a curious corner case that can happen when, for example, you're reading the length-value encoded strings in OpenSSH. If the last string in the data is of zero-length, then decode() will be called with an empty buffer, so we don't need to read any bytes. However, since
is_finished()was checked unconditionally, the zero-length read (which would be OK) was instead rejected because there was no data to (not) read.The bug was particularly mischievous because
is_finished()will return false if there's=padding, so 2/3 of empty read calls will work normally.