Fix an out-of-bounds read for TLS <- 1.2 cipher strings that end in a "-" #19166
Conversation
millert
changed the title
paulidale
added
branch: master
t8m
added
the
approval: done
|
Confirm trivial? I think it is but I added the label 😃 |
|
I am OK with CLA: trivial for this. @paulidale should this go into 1.1.1 too? IMO yes, if it applies there. |
|
Should this have a regression test? That test combined with ASan should provide regression coverage. |
ssl/ssl_ciph.c
Outdated
| @@ -1064,7 +1064,8 @@ static int ssl_cipher_process_rulestr(const char *rule_str, | |||
| */ | |||
| ERR_raise(ERR_LIB_SSL, SSL_R_INVALID_COMMAND); | |||
| retval = found = 0; | |||
| l++; | |||
| if (ch != '\0') | |||
| l++; | |||
There was a problem hiding this comment.
Actually, is this the right fix? This is an error condition, ERR_raise and all, and yet we're breaking out of just the inner loop. It seems we actually just want to break out of the outer loop.
Looking back, it seems we hit this with a fuzzer a while back, but our fix was to make the function immediately fail.
https://boringssl-review.googlesource.com/c/boringssl/+/11421/
There was a problem hiding this comment.
Returning early seems safer.
There was a problem hiding this comment.
Yeah. It also seems the function will get confused otherwise. If I'm reading this right, I believe break exits to line 1220 ("Ok, we have the rule, now apply it"). That could try to execute CIPHER_SPECIAL or skip over something and then continue running. We may execute more rules, pile on more errors, before finally returning failure anyway because retval is cleared.
|
For some reason this can be hard to trigger with address sanitizer. The following reliably reproduces the problem for me with |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
t8m
added
branch: 3.1
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial
|
@t8m I updated the commit to just return early as discussed. |
t8m
added
the
approval: review pending
|
@paulidale still OK? |
paulidale
added
approval: done
openssl-machine
removed
the
approval: done
openssl-machine
added
the
approval: ready to merge
|
This pull request is ready to merge |
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19166)
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19166) (cherry picked from commit 428511c)
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19166) (cherry picked from commit 428511c)
|
Merged to all branches (after fixing trivial conflict on 1.1.1 branch). Thank you for your contribution. |
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19166) (cherry picked from commit 428511c)
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19166) (cherry picked from commit 428511c)
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19166) (cherry picked from commit 428511c)
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19166) (cherry picked from commit 428511c)
If rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. It is safest to just return early in this case since the condition occurs inside a nested loop. CLA: trivial Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19166)
In ssl_cipher_process_rulestr(), if rule_str ended in a "-", "l" was incremented one byte past the end of the buffer. This resulted in an out-of-bounds read when "l" is dereferenced at the end of the loop. The trivial fix is to only increment "l" if the last byte read is not a NUL.
CLA: trivial