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

Fix DSA/ECDSA sign infinite loop(s) #20384

Closed
wants to merge 5 commits into from
Closed

Fix DSA/ECDSA sign infinite loop(s) #20384

wants to merge 5 commits into from

Conversation

slontis
Copy link
Member

@slontis slontis commented Feb 27, 2023

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis
Fix infinite loops in DSA sign code.
dab3b3b 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.
@slontis
fixup! Fix infinite loops in DSA sign code.
03b50f9
@slontis
fixup! Fix infinite loops in DSA sign code.
eb27fe6
@slontis
Fix potential infinite loops in ECDSA signing.
aa91cec 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.
@slontis
fixup! Fix potential infinite loops in ECDSA signing.
d4a120a
@slontis slontis added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: present The PR has suitable tests present labels Feb 27, 2023
@slontis
Copy link
Member Author

slontis commented Feb 27, 2023

I assume this should not go into 3.1

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Feb 27, 2023
paulidale
paulidale approved these changes Feb 27, 2023
View reviewed changes
Copy link
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 don't see why this shouldn't go to 3.1 or even 3.0.
It's fixing a bug that results in a DoS.

@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Feb 27, 2023
@slontis
Copy link
Member Author

slontis commented Feb 27, 2023 •
edited

DSA (signing) is not part of FIPS 186-5

@mattcaswell
Copy link
Member

DSA (signing) is not part of FIPS 186-5

But doesn't this impact the default provider?

@mattcaswell mattcaswell 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 Feb 27, 2023
@slontis slontis added branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Feb 27, 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 Feb 28, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Feb 28, 2023
@slontis @paulidale
Fix infinite loops in DSA sign code.
3a4e09a 
Fixes #20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20384)
openssl-machine pushed a commit that referenced this pull request Feb 28, 2023
@slontis @paulidale
Fix potential infinite loops in ECDSA signing.
5f820bd 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20384)
openssl-machine pushed a commit that referenced this pull request Feb 28, 2023
@slontis @paulidale
Fix infinite loops in DSA sign code.
dda8b03 
Fixes #20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20384)

(cherry picked from commit 3a4e09a)
openssl-machine pushed a commit that referenced this pull request Feb 28, 2023
@slontis @paulidale
Fix infinite loops in DSA sign code.
5a53d73 
Fixes #20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20384)

(cherry picked from commit 3a4e09a)
@paulidale
Copy link
Contributor

Merged with manual fixes for 3.0 and 3.1

@paulidale paulidale closed this Feb 28, 2023
openssl-machine pushed a commit that referenced this pull request Feb 28, 2023
@slontis @paulidale
Fix potential infinite loops in ECDSA signing.
c13dc37 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
openssl-machine pushed a commit that referenced this pull request Feb 28, 2023
@slontis @paulidale
Fix potential infinite loops in ECDSA signing.
2022b9e 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20384)

(cherry picked from commit 5f820bd)
@bernd-edlinger
Copy link
Member

Is there a reason why this should not be back-ported to 1.1.1 ?

bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 2, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
5ebd8ff 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 2, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
f4160d6 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 2, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
f4413ef 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 13, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
3307887 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 13, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
2ae4651 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 17, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
188c808 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 17, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
23ce934 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 30, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
ed436cf 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request May 30, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
942f5f5 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jul 19, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
7b1f02e 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jul 19, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
10cc6b1 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 1, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
32c3c3f 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 1, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
b6730ef 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 19, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
fc83ad9 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 19, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
993a4b6 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2023
@slontis @bernd-edlinger
Fix infinite loops in DSA sign code.
6a84eb7 
Fixes openssl#20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 3a4e09a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2023
@slontis @bernd-edlinger
Fix potential infinite loops in ECDSA signing.
a164c97 
Similiar checks to the DSA code have been added for ECDSA also.
This should not be a problem when using named groups.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#20384)

(cherry picked from commit 5f820bd)
(cherry picked from commit 2022b9e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@slontis @mattcaswell @openssl-machine @paulidale @bernd-edlinger