Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bin2bn(): When len==0, just return a zero BIGNUM #20033

Closed
wants to merge 5 commits into from
Closed

bin2bn(): When len==0, just return a zero BIGNUM #20033

wants to merge 5 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Jan 12, 2023

This allows calls with s==NULL and len==0 to be safe. It probably already
was, but address sanitizers could still complain.

This fixes the underlying problem detected in #20011

@levitte
bin2bn(): When len==0, just return a zero BIGNUM
31a295c 
This allows calls with s==NULL and len==0 to be safe.  It probably already
was, but address sanitizers could still complain.
@levitte levitte added approval: otc review pending This pull request needs review by an OTC member approval: review pending This pull request needs review by a committer branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Jan 12, 2023
@levitte levitte self-assigned this Jan 12, 2023
@levitte levitte requested review from a team January 12, 2023 09:27
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 12, 2023
@levitte levitte mentioned this pull request Jan 12, 2023
Closed
@levitte
Copy link
Member Author

levitte commented Jan 12, 2023

This replaces #20017

@levitte
Copy link
Member Author

levitte commented Jan 12, 2023

Something to be noted is that this isn't necessary in 3.1 or 3.0, as they have older code, where this is already correctly dealt with. It can be argued that this fixes an omission from the refactoring that happened in master

@t8m
Copy link
Member

t8m commented Jan 12, 2023

Would it be possible to add a testcase?

@t8m t8m added the hold: needs tests The PR needs tests to be added to it label Jan 12, 2023
@ifranzki
Copy link
Contributor

Something to be noted is that this isn't necessary in 3.1 or 3.0, as they have older code, where this is already correctly dealt with. It can be argued that this fixes an omission from the refactoring that happened in master

This explains why I saw the address sanitizer complaining about this just now (when running my provider against master), although the bug in my provider leading to that situation was there much longer....

@levitte
Add a test for public variants of bn2bin()
a9c239b 
We test with binary input of length 1, length 0, and NULL input with length 0
@levitte
Copy link
Member Author

levitte commented Jan 12, 2023

Test added... I wonder if that should be backported to 3.1 and 3.0

@levitte levitte added tests: present The PR has suitable tests present and removed hold: needs tests The PR needs tests to be added to it labels Jan 12, 2023
@t8m
Copy link
Member

t8m commented Jan 12, 2023

Hmm... CI is relevant

@t8m
Copy link
Member

t8m commented Jan 12, 2023

Test added... I wonder if that should be backported to 3.1 and 3.0

+1 to that.

@levitte
Copy link
Member Author

levitte commented Jan 12, 2023

Hmm... CI is relevant

Oops, forgot to free zbn in the test. Fixed

tmshort
tmshort approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tmshort tmshort removed the approval: review pending This pull request needs review by a committer label Jan 12, 2023
slontis
slontis reviewed Jan 12, 2023
View reviewed changes
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that this function also blindly allows len to be negative.. (Which seems bad to me)

paulidale
paulidale requested changes Jan 13, 2023
View reviewed changes
crypto/bn/bn_lib.c Show resolved Hide resolved
test/bntest.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member Author

levitte commented Jan 13, 2023

I notice that this function also blindly allows len to be negative.. (Which seems bad to me)

True that... and looking back at released versions, I see the same there, all the way down to 1.1.1. That might be a subject for a different PR

@levitte
Copy link
Member Author

levitte commented Jan 13, 2023

I added code to catch the len < 0 problem, and a test for it

@levitte levitte requested review from tmshort and t8m January 13, 2023 11:53
@levitte levitte added the approval: review pending This pull request needs review by a committer label Jan 13, 2023
@levitte
Copy link
Member Author

levitte commented Jan 13, 2023

Re-review needed

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jan 13, 2023
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 17, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 18, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member

hlandau commented Jan 20, 2023

Merged to master. Thank you.

@hlandau hlandau closed this Jan 20, 2023
openssl-machine pushed a commit that referenced this pull request Jan 20, 2023
@levitte @hlandau
bin2bn(): When len==0, just return a zero BIGNUM
1b24b5a 
This allows calls with s==NULL and len==0 to be safe.  It probably already
was, but address sanitizers could still complain.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20033)
openssl-machine pushed a commit that referenced this pull request Jan 20, 2023
@levitte @hlandau
Add a test for public variants of bn2bin()
1519233 
We test with binary input of length 1, length 0, and NULL input with length 0

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20033)
openssl-machine pushed a commit that referenced this pull request Jan 20, 2023
@levitte @hlandau
bn2bin(): Don't accept len < 0
c9466f3 
Test included

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20033)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slontis slontis slontis left review comments

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

@tmshort tmshort Awaiting requested review from tmshort

Assignees

@levitte levitte

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@levitte @t8m @ifranzki @openssl-machine @hlandau @tmshort @paulidale @slontis