Negative test case for constructed, definite-length octet string #1931
Conversation
|
This is incomplete, but here’s an attempt at ensuring that constructed, definite-length method encoded octet strings are detected and rejected. Seeking feedback; I made a few attempts but finally realized what you, @dishmaker, meant by your comment As the I thought it might be fair game to draft a change to the |
dwhjames
force-pushed
the
constructed_definite_octet_string
branch
from
94e1198 to
78a3118
Compare
|
Propagating the original |
| @@ -13,6 +13,8 @@ pub struct Header { | |||
|
|
|||
| /// Length of the encoded value | |||
| pub length: Length, | |||
|
|
|||
| constructed: bool, | |||
There was a problem hiding this comment.
Hmm, not sure how I feel about the mixed visibility here. Maybe make it pub for now and perhaps we could have a followup to make all of the fields private? (I imagine that would be a bit more invasive)
There was a problem hiding this comment.
Yep, definitely ugly.
Revised in 544e397
and perhaps we could have a followup to make all of the fields private? (I imagine that would be a bit more invasive)
I counted 24 occurrences in the der crate; for now I also added accessor methods so that new code has the option to use them in the event that the fields become private.
There was a problem hiding this comment.
Maybe just go through with it, if it seems mostly internal to the der crate
There was a problem hiding this comment.
I went through with it: 0c5789e
However, tests are failing due to at least
https://github.com/RustCrypto/signatures/blob/1ff759e67a1675d759e901fdb5903a07a1d7caec/ecdsa/src/der.rs#L206-L210
It looks like this change would have to be phased in?
Should I
- roll back all the changes for using accessor methods rather than directly accessing struct fields, OR
- roll back making the fields private but keep the pattern of using accessor methods rather than direct field access
?
There was a problem hiding this comment.
Looks great so far! Easier than I expected, actually
You can open a PR to https://github.com/RustCrypto/signatures which makes the necessary changes, using a [patch.crates-io] directive to pull in this git branch.
After that, in a bit of an ouroboros you can then update this PR with a [patch.crates-io] directive to pull in ecdsa from your fork/branch of https://github.com/RustCrypto/signatures. That should get the tests passing.
There was a problem hiding this comment.
Ok, I think I followed your guidance
dwhjames
force-pushed
the
constructed_definite_octet_string
branch
from
78a3118 to
544e397
Compare
dwhjames
force-pushed
the
constructed_definite_octet_string
branch
from
544e397 to
0c5789e
Compare
der/src/header.rs
Outdated
| /// Create a new [`Header`] from a [`Tag`] and a specified length. | ||
| /// | ||
| /// Returns an error if the length exceeds the limits of [`Length`]. | ||
| pub fn new(tag: Tag, length: impl TryInto<Length>) -> Result<Self> { | ||
| pub fn try_new(tag: Tag, length: impl TryInto<Length>) -> Result<Self> { |
There was a problem hiding this comment.
Hmm, does this have any users? I wonder if it could be removed
There was a problem hiding this comment.
It does not; I initially thought it best to preserve the flexibility of the original, but there seem to be no current/remaining cases of leveraging the TryInto.
Removed in 8d82e78
der/src/header.rs
Outdated
| } | ||
|
|
||
| /// Copy of header with adjusted length. | ||
| pub fn copy_with_length(&self, length: Length) -> Self { |
There was a problem hiding this comment.
Sure; changed to with_length in 8d82e78
dwhjames
force-pushed
the
constructed_definite_octet_string
branch
2 times, most recently
from
fa5700c to
0d37f57
Compare
Ensure octet strings with constructed, definite-length method return an error, rather than an incorrect decoding.
dwhjames
force-pushed
the
constructed_definite_octet_string
branch
from
0d37f57 to
86da1ac
Compare
Ensure octet strings with constructed, definite-length method return an error, rather than an incorrect decoding.