Avoid division by zero in hybrid point encoding#15108
Avoid division by zero in hybrid point encoding#15108botovq wants to merge 0 commit intoopenssl:masterfrom
Conversation
openssl-machine
added
the
hold: cla required
|
As mentioned in #15021, I am going to sign the CLA. |
romen
added
branch: master
romen
left a comment
romen
left a comment
There was a problem hiding this comment.
LGTM
Thanks for also including a test!
t8m
added
the
approval: done
|
Waiting for the CLA to be signed. |
botovq
force-pushed
the
point_conversion
branch
2 times, most recently
from
36ec123 to
d4d606e
Compare
|
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
approval: done
|
Have you submitted the CLA already? |
|
This will need make update after rebasing before merge as it touches FIPS module sources. |
|
@t8m No, not yet. As mentioned in the linked issue, it might take a week
or two before I get access to a printer or scanner. I'll do it as soon
as I can.
Do I need to do 'make update' or is this something the person who merges
will do?
|
@t8m Does this mean that we need to rescope what is considered beta1 blocker? |
I do not think so. We never said that post beta1 no FIPS relevant source changes are allowed. I suppose we might want to restrict changes to them some time later during the beta cycle but it should be definitely allowed for some while after beta1. |
I think it can be done by the person who merges but of course feel free to do it yourself if you want. It should also fix the check_update test failure. |
|
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. |
I know that people with neither have been able to do some kind of electronic-only thing in the past, but I don't handle CLA processing so I don't know the details of how :( |
openssl-machine
removed
the
hold: cla required
|
@botovq I will deal with the |
In hybrid and compressed point encodings, the form octet contains a bit of information allowing to calculate y from x. For a point on a binary curve, this bit is zero if x is zero, otherwise it must match the rightmost bit of of the field element y / x. The existing code only considers the second possibility. It could thus incorrecly fail with a division by zero error as found by Guido Vranken's cryptofuzz. This commit adds a few explanatory comments to oct2point. The only actual code change is in the last hunk which adds a BN_is_zero(x) check to avoid the division by zero. Fixes #15021 Reviewed-by: Nicola Tuveri <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15108)
Reviewed-by: Nicola Tuveri <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15108)
Reviewed-by: Tomas Mraz <[email protected]> (Merged from #15108)
|
Merged to
Thanks @botovq for the contribution and @guidovranken for raising the issue! |
|
@botovq I tried force pushing to your branch to see if GitHub would recognize the PR as Unfortunately I pushed to |
If you have a digitized copy of your signature already, free PDF tools can add it into an existing PDF. Or word-processing programs can also do it. That might helps. |
|
On Mon, May 10, 2021 at 07:14:17AM -0700, Rich Salz wrote:
> access to a printer or scanner
If you have a digitized copy of your signature already, free PDF tools can add it into an existing PDF. Or word-processing programs can also do it. That might helps.
Thanks for the suggestion. I sent the CLA on Friday and it was processed
yesterday, so the problem is solved :)
|
In hybrid and compressed point encodings, the form octet contains a bit of information allowing to calculate y from x. For a point on a binary curve, this bit is zero if x is zero, otherwise it must match the rightmost bit of of the field element y / x. The existing code only considers the second possibility. It could thus incorrecly fail with a division by zero error as found by Guido Vranken's cryptofuzz. This commit adds a few explanatory comments to oct2point. The only actual code change is in the last hunk which adds a BN_is_zero(x) check to avoid the division by zero. Fixes openssl#15021 Reviewed-by: Nicola Tuveri <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15108)
Reviewed-by: Nicola Tuveri <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15108)
Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#15108)
7 participants
In hybrid and compressed point encodings, the form octet contains a bit
of information allowing to calculate y from x. For a point on a binary
curve, this bit is zero if x is zero, otherwise it must match the
rightmost bit of of the field element y / x. The existing code only
considers the second possibility. It could thus incorrecly fail with a
division by zero error as found by Guido Vranken's cryptofuzz.
This commit adds a few explanatory comments to oct2point. The only
actual code change is in the last hunk which adds a BN_is_zero(x)
check to avoid the division by zero.
Checklist