Skip to content

Fix OSSL_PARAM_allocate_from_text() to do sign extension#17104

Closed
levitte wants to merge 4 commits intoopenssl:masterfrom
levitte:fix-OSSL_PARAM_allocate_from_text-20211122
Closed

Fix OSSL_PARAM_allocate_from_text() to do sign extension#17104
levitte wants to merge 4 commits intoopenssl:masterfrom
levitte:fix-OSSL_PARAM_allocate_from_text-20211122

Conversation

@levitte
Copy link
Copy Markdown
Member

@levitte levitte commented Nov 22, 2021

OSSL_PARAM_allocate_from_text() doesn't do sign extension when it should.
See #17103.

…size ints

With arbitrary size ints, we get to know exactly how large the minimum
buffer must be.
…mber

When the parameter definition has the data type OSSL_PARAM_UNSIGNED_INTEGER,
negative input values should not be accepted.

Fixes openssl#17103
This is done for the data type OSSL_PARAM_INTEGER by checking if the
most significant bit is set, and adding 8 to the number of buffer bits
if that is the case.  Everything else is already in place.

Fixes openssl#17103
@levitte levitte added branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch approval: review pending This pull request needs review by a committer labels Nov 22, 2021
@Angelika123456
Copy link
Copy Markdown

OSSL_PARAM_allocate_from_text() doesn't do sign extension when it should. See #17103.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 22, 2021
Copy link
Copy Markdown
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

I like.

@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 Nov 22, 2021
@paulidale
Copy link
Copy Markdown
Contributor

This looks like a feature, so not for 3.0. Anyone think differently?

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Nov 23, 2021

Go look at #17103, then decide if you think this is a bugfix or an added feature.

(I think it's a bugfix)

@paulidale paulidale added the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Nov 23, 2021
@paulidale
Copy link
Copy Markdown
Contributor

OTC question: bug fix or feature?

@mattcaswell
Copy link
Copy Markdown
Member

OTC:Bug fix

@mattcaswell mattcaswell removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Nov 23, 2021
@paulidale paulidale 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 Nov 23, 2021
openssl-machine pushed a commit that referenced this pull request Nov 24, 2021
…size ints

With arbitrary size ints, we get to know exactly how large the minimum
buffer must be.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17104)
openssl-machine pushed a commit that referenced this pull request Nov 24, 2021
…mber

When the parameter definition has the data type OSSL_PARAM_UNSIGNED_INTEGER,
negative input values should not be accepted.

Fixes #17103

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17104)
openssl-machine pushed a commit that referenced this pull request Nov 24, 2021
This is done for the data type OSSL_PARAM_INTEGER by checking if the
most significant bit is set, and adding 8 to the number of buffer bits
if that is the case.  Everything else is already in place.

Fixes #17103

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17104)
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Nov 24, 2021

Merged

master:
b556713 Test the performance of OSSL_PARAM_allocate_from_text with arbitrary size ints
8585b5b Have OSSL_PARAM_allocate_from_text() raise error on unexpected neg number
946bc0e Allow sign extension in OSSL_PARAM_allocate_from_text()

3.0:
b417590 Test the performance of OSSL_PARAM_allocate_from_text with arbitrary size ints
de54b77 Have OSSL_PARAM_allocate_from_text() raise error on unexpected neg number
f838730 Allow sign extension in OSSL_PARAM_allocate_from_text()

@levitte levitte closed this Nov 24, 2021
@levitte levitte deleted the fix-OSSL_PARAM_allocate_from_text-20211122 branch November 24, 2021 18:22
openssl-machine pushed a commit that referenced this pull request Nov 24, 2021
…size ints

With arbitrary size ints, we get to know exactly how large the minimum
buffer must be.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17104)

(cherry picked from commit b556713)
openssl-machine pushed a commit that referenced this pull request Nov 24, 2021
…mber

When the parameter definition has the data type OSSL_PARAM_UNSIGNED_INTEGER,
negative input values should not be accepted.

Fixes #17103

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17104)

(cherry picked from commit 8585b5b)
openssl-machine pushed a commit that referenced this pull request Nov 24, 2021
This is done for the data type OSSL_PARAM_INTEGER by checking if the
most significant bit is set, and adding 8 to the number of buffer bits
if that is the case.  Everything else is already in place.

Fixes #17103

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17104)

(cherry picked from commit 946bc0e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch severity: fips change The pull request changes FIPS provider sources

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants