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

punycode: use WPACKET for building the result #19591

Closed
wants to merge 7 commits into from
Closed

punycode: use WPACKET for building the result #19591

wants to merge 7 commits into from

Conversation

paulidale
Copy link
Contributor

@paulidale paulidale commented Nov 2, 2022 •
edited

Removing the recently added bounds checking code in favour of our WPACKET code.

  • Added a unit test that fails before the recent CVE fix & passes with it. This PR isn't changing the behaviour per say.

  • Added a notional number of bytes that were attempted to be written to WPACKET, so the required space return is possible.

  • Converted the punycode code to use WPACKET.

  • Added a fuzzer

  • documentation is added or updated

  • tests are added or updated

@paulidale paulidale 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 triaged: refactor The issue/pr requests/implements refactoring branch: 3.1 Merge to openssl-3.1 labels Nov 2, 2022
@paulidale paulidale self-assigned this Nov 2, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 2, 2022
@thesamesam thesamesam mentioned this pull request Nov 2, 2022
Open
@paulidale paulidale force-pushed the punycodepacket branch 3 times, most recently from dee6213 to 3a6b349 Compare November 3, 2022 00:57
@paulidale
Copy link
Contributor Author

We might want to cherry pick the x509 commit to 3.0.
Otherwise, we're accepting strings one character fewer than previously.

t8m
t8m reviewed Nov 3, 2022
View reviewed changes
crypto/packet.c Outdated Show resolved Hide resolved
t8m
t8m reviewed Nov 3, 2022
View reviewed changes
crypto/punycode.c Outdated Show resolved Hide resolved
@paulidale paulidale marked this pull request as draft November 3, 2022 08:08
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Nov 3, 2022
@paulidale paulidale marked this pull request as ready for review November 3, 2022 09:36
t8m
t8m reviewed Nov 3, 2022
View reviewed changes
test/punycode_test.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Nov 3, 2022

Approved assuming CI passes.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Nov 3, 2022
@mspncp
Copy link
Contributor

mspncp commented Nov 3, 2022

Would it be asked too much to add a simple fuzzing test, as suggested by @hannob on Twitter?

@paulidale
Copy link
Contributor Author

In a separate PR ?

@thesamesam
Copy link
Contributor

thesamesam commented Nov 3, 2022 •
edited

I'd go for a separate PR as some tinkering might be needed to make CI happy and it's not related to the substance of this change, even though it's related to the CVE.

@mspncp
Copy link
Contributor

mspncp commented Nov 3, 2022

In a separate PR ?

A separate PR is ok, as long as it happens. We should not make the same mistake twice.

mspncp
mspncp reviewed Nov 3, 2022
View reviewed changes
fuzz/punycode.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Nov 4, 2022 •
edited

I did mention the weight of WPACKET and my concerns were dismissed out of hand. Hence this PR.
Can't have it both ways. Either it's heavy or it isn't.

I've changed my mind since that time - it is a little bit heavy - could we make it leaner?

@paulidale
Copy link
Contributor Author

I'd need to write an overflow safe buffer library that doesn't do any memory allocation (i.e. it isn't WPACKET but something new). I'd leave out the sub packet processing etc. Essentially it would be append only with memcpy, strcpy, put byte, get length & probably a few others.

The punycode decoder requires an insert operations and I'm not currently doing this with WPACKET because it's not supported. Bad range checking in this code was one of the CVEs 😞

@slontis
Copy link
Member

slontis commented Nov 7, 2022

I did mention the weight of WPACKET and my concerns were dismissed out of hand. Hence this PR. Can't have it both ways. Either it's heavy or it isn't.

You did not explain before why it was heavy..

@slontis
Copy link
Member

slontis commented Nov 7, 2022

I've added some fuzzing. If the tests are happy, I'll leave it. If not I'll move it to a new PR.

Can you do a quick test of this against the old dodgy code as well?

@paulidale
Copy link
Contributor Author

The fuzzing does crash the old code. See the two corpora I already included.

@paulidale
Copy link
Contributor Author

You did not explain before why it was heavy..

I rather suspect this is getting the onus backwards.

@paulidale
Copy link
Contributor Author

This has one zalloc overhead. Making it leaner is going to require quite a bit of effort. I'm not convinced it worthwhile especially given the heaviness of certificate processing where it occurs.

@slontis
Copy link
Member

slontis commented Nov 8, 2022

If it was added I would expect that WPACKET might use the lightweight layer. (Having any type of error in the new code would not be a good thing though :))

@thesamesam
Copy link
Contributor

thesamesam commented Nov 8, 2022 •
edited

Is it worth trying to do some benchmarks and see if there's an actual problem? Some of the previous problems have been because of determination to increase performance but that's not always the right tradeoff (see e.g. the AVX512 discussion we're having).

Sometimes safety costs a bit more and there's already been a problem in the area this PR is designed to solve, i.e. this is obviously going to be a bit slower, but is it problematically slower?

Could also maybe let it sit in master and see how things go, but not backport it.

@t8m
Copy link
Member

t8m commented Nov 8, 2022

Could also maybe let it sit in master and see how things go, but not backport it.

There is currently no intention to backport it to 3.0 as this is a refactoring not a bug fix.

@t8m
Copy link
Member

t8m commented Nov 8, 2022

This has one zalloc overhead. Making it leaner is going to require quite a bit of effort. I'm not convinced it worthwhile especially given the heaviness of certificate processing where it occurs.

This surely can be merged without changing WPACKET. This is not performance critical peace of code anyway. It's just that if we want to use WPACKET truly everywhere we should first consider to make it leaner for the cases where the output buffer is static and there is no need for subpacket support.

@paulidale
Copy link
Contributor Author

It needs approvals before it can merge...

t8m
t8m requested changes Nov 8, 2022
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Just doc nits

crypto/punycode.c Outdated Show resolved Hide resolved
doc/internal/man3/ossl_punycode_decode.pod Outdated Show resolved Hide resolved
doc/internal/man3/ossl_punycode_decode.pod Outdated Show resolved Hide resolved
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Nov 9, 2022
@paulidale
Copy link
Contributor Author

Ping for second review.

beldmit
beldmit approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit 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 9, 2022
openssl-machine pushed a commit that referenced this pull request Nov 10, 2022
@paulidale
punycode: update to use WPACKET instead of using custom range checking
905ba92 
Add test for `.' overflows, remove the output size argument from
ossl_a2ulabel() since it was never used and greatly complicated the code.
Convert ossl_a2ulabel() to use WPACKET for building the output string.
Update the documentation to match the new definition of ossl_a2ulabel().

x509: let punycode handle the '\0' string termination.  Saves a memset(3)
and some size fiddling.  Also update to deal with the modified parameters.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19591)
openssl-machine pushed a commit that referenced this pull request Nov 10, 2022
@paulidale
fuzz: add punycode decoder fuzz test
8aa82b3 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19591)
@paulidale
Copy link
Contributor Author

merged.

@paulidale paulidale closed this Nov 10, 2022
@paulidale paulidale deleted the punycodepacket branch November 10, 2022 21:23
openssl-machine pushed a commit that referenced this pull request Nov 10, 2022
@paulidale
punycode: update to use WPACKET instead of using custom range checking
709c04b 
Add test for `.' overflows, remove the output size argument from
ossl_a2ulabel() since it was never used and greatly complicated the code.
Convert ossl_a2ulabel() to use WPACKET for building the output string.
Update the documentation to match the new definition of ossl_a2ulabel().

x509: let punycode handle the '\0' string termination.  Saves a memset(3)
and some size fiddling.  Also update to deal with the modified parameters.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19591)

(cherry picked from commit 905ba92)
openssl-machine pushed a commit that referenced this pull request Nov 10, 2022
@paulidale
fuzz: add punycode decoder fuzz test
f01ebab 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #19591)

(cherry picked from commit 8aa82b3)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@paulidale @beldmit
punycode: update to use WPACKET instead of using custom range checking
72737b5 
Add test for `.' overflows, remove the output size argument from
ossl_a2ulabel() since it was never used and greatly complicated the code.
Convert ossl_a2ulabel() to use WPACKET for building the output string.
Update the documentation to match the new definition of ossl_a2ulabel().

x509: let punycode handle the '\0' string termination.  Saves a memset(3)
and some size fiddling.  Also update to deal with the modified parameters.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from openssl#19591)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@paulidale @beldmit
fuzz: add punycode decoder fuzz test
edf8ad9 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from openssl#19591)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kroeckx kroeckx kroeckx left review comments

@mspncp mspncp mspncp left review comments

@thesamesam thesamesam thesamesam left review comments

@t8m t8m t8m approved these changes

@beldmit beldmit beldmit approved these changes

Assignees

@paulidale paulidale

Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 3.1 Merge to openssl-3.1 triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@paulidale @t8m @mspncp @thesamesam @slontis @kroeckx @beldmit