acw/simple_asn1
1
0
Fork 0
Code Issues 5 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

[security] Fix a panic from an unchecked string slice. #28

Merged
nmathewson merged 1 commits from fix_string_slice into master 2021-11-14 13:45:34 -08:00
Conversation 7 Commits 1 Files Changed 1 +16 -1
nmathewson commented 2021-11-14 06:48:11 -08:00 (Migrated from github.com)
Copy Link

When slicing a string, you get a panic if you do so at any point
other than at a character boundary. This happened in the
implementation of UTCTime parsing.

This bug was introduced in bc156c36d7,
and appears to affect only version 0.6.0.

I've tried using the clippy::string_slice lint to confirm that there
are not any other string slices in this code.

Fixes bug #27. Found via fuzzing.

When slicing a string, you get a panic if you do so at any point other than at a character boundary. This happened in the implementation of UTCTime parsing. This bug was introduced in bc156c36d76d91b77cdf8f1f231e2c554ea0e454, and appears to affect only version 0.6.0. I've tried using the clippy::string_slice lint to confirm that there are not any other string slices in this code. Fixes bug #27. Found via fuzzing.
nmathewson commented 2021-11-14 12:41:46 -08:00 (Migrated from github.com)
Copy Link

I fuzzed this branch for a couple more hours, but no further problems turned up.

I fuzzed this branch for a couple more hours, but no further problems turned up.
nmathewson commented 2021-11-14 12:46:39 -08:00 (Migrated from github.com)
Copy Link

By the way, please let me know if you'd like me to open a RUSTSEC advisory for this, or if you'd rather do it yourself?

By the way, please let me know if you'd like me to open a RUSTSEC advisory for this, or if you'd rather do it yourself?
acw commented 2021-11-14 13:48:10 -08:00 (Migrated from github.com)
Copy Link

Hey, you found it, I think you should get the credit for the advisory :)

I'll put another comment in here with the new version number (which will almost certainly be 0.6.1, but ...).

Hey, you found it, I think you should get the credit for the advisory :) I'll put another comment in here with the new version number (which will almost certainly be 0.6.1, but ...).
acw commented 2021-11-14 13:54:26 -08:00 (Migrated from github.com)
Copy Link

Published upstream as 0.6.1!

Published upstream as 0.6.1!
Shnatsel commented 2021-11-14 18:16:33 -08:00 (Migrated from github.com)
Copy Link

Hello, RustSec maintainer here. Thanks for acting on the issue so quickly!

I'd like to confirm with @acw that you want to treat panics in this crate as security issues. It's reasonable for a format decoder that handles untrusted data to treat panics as a security issue; but we don't want to have an inconsistent coverage where some panics in the crate are tracked but others are not.

Hello, RustSec maintainer here. Thanks for acting on the issue so quickly! I'd like to confirm with @acw that you want to treat panics in this crate as security issues. It's reasonable for a format decoder that handles untrusted data to treat panics as a security issue; but we don't want to have an inconsistent coverage where some panics in the crate are tracked but others are not.
acw commented 2021-11-17 16:19:37 -08:00 (Migrated from github.com)
Copy Link

After some thinking: yes, I think that makes sense. Especially for something like ASN.1, being able to crash a process remotely could be a significant safety or security concern.

After some thinking: yes, I think that makes sense. Especially for something like ASN.1, being able to crash a process remotely could be a significant safety or security concern.
Shnatsel commented 2021-11-17 16:29:03 -08:00 (Migrated from github.com)
Copy Link

I'm merging the advisory then. Thanks again!

I'm merging the advisory then. Thanks again!
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
acw
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: acw/simple_asn1#28
Delete Branch "fix_string_slice"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?