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.

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

jwt: reduce heap allocations in jwt.Parse #3403

Merged
merged 2 commits into from
Feb 10, 2025
Merged

Conversation

jub0bs
Copy link
Contributor

@jub0bs jub0bs commented Feb 10, 2025

Function jwt.Parse currently splits (via a call to strings.Split) its argument (which is untrusted data) on periods.

As a result, in the face of a malicious request whose Authorization header consists of Bearer followed by many period characters, a call to that function incurs allocations to the tune of O(n) bytes (where n stands for the length of the function's argument), with a constant factor of about 16. Relevant weakness: CWE-405: Asymmetric Resource Consumption (Amplification)

With this change, jwt.Parse allocates O(1) bytes even in the face of such malicious requests.


Some benchmark results:

goos: darwin
goarch: amd64
pkg: github.com/zalando/skipper/jwt
cpu: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
                                       │        old        │                  new                  │
                                       │      sec/op       │    sec/op     vs base                 │
Parse_malicious/all_periods-8            13681180.50n ± 2%   97.85n ± 47%  -100.00% (p=0.000 n=10)
Parse_malicious/two_trailing_periods-8         60.78µ ± 1%   35.41µ ± 11%   -41.74% (p=0.000 n=10)
geomean                                        911.9µ        1.861µ         -99.80%

                                       │       old        │                 new                 │
                                       │       B/op       │    B/op     vs base                 │
Parse_malicious/all_periods-8            16785409.00 ± 0%   64.00 ± 0%  -100.00% (p=0.000 n=10)
Parse_malicious/two_trailing_periods-8         224.0 ± 0%   240.0 ± 0%    +7.14% (p=0.000 n=10)
geomean                                      59.88Ki        123.9        -99.80%

                                       │    old     │                 new                 │
                                       │ allocs/op  │ allocs/op   vs base                 │
Parse_malicious/all_periods-8            1.000 ± 0%   1.000 ± 0%       ~ (p=1.000 n=10) ¹
Parse_malicious/two_trailing_periods-8   4.000 ± 0%   4.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                  2.000        2.000       +0.00%
¹ all samples are equal

Signed-off-by: jub0bs <jcretel-infosec+github@protonmail.com>
Some benchmark results:

```
goos: darwin
goarch: amd64
pkg: github.com/zalando/skipper/jwt
cpu: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
                                       │        old        │                  new                  │
                                       │      sec/op       │    sec/op     vs base                 │
Parse_malicious/all_periods-8            13681180.50n ± 2%   97.85n ± 47%  -100.00% (p=0.000 n=10)
Parse_malicious/two_trailing_periods-8         60.78µ ± 1%   35.41µ ± 11%   -41.74% (p=0.000 n=10)
geomean                                        911.9µ        1.861µ         -99.80%

                                       │       old        │                 new                 │
                                       │       B/op       │    B/op     vs base                 │
Parse_malicious/all_periods-8            16785409.00 ± 0%   64.00 ± 0%  -100.00% (p=0.000 n=10)
Parse_malicious/two_trailing_periods-8         224.0 ± 0%   240.0 ± 0%    +7.14% (p=0.000 n=10)
geomean                                      59.88Ki        123.9        -99.80%

                                       │    old     │                 new                 │
                                       │ allocs/op  │ allocs/op   vs base                 │
Parse_malicious/all_periods-8            1.000 ± 0%   1.000 ± 0%       ~ (p=1.000 n=10) ¹
Parse_malicious/two_trailing_periods-8   4.000 ± 0%   4.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                  2.000        2.000       +0.00%
¹ all samples are equal
```

Signed-off-by: jub0bs <jcretel-infosec+github@protonmail.com>
@AlexanderYastrebov AlexanderYastrebov added the bugfix Bug fixes and patches label Feb 10, 2025
@AlexanderYastrebov
Copy link
Member

Thanks!

@AlexanderYastrebov
Copy link
Member

👍

1 similar comment
@MustafaSaber
Copy link
Member

👍

@MustafaSaber MustafaSaber merged commit 30f0005 into zalando:master Feb 10, 2025
10 checks passed
AlexanderYastrebov added a commit that referenced this pull request Feb 11, 2025
Use strings.LastIndex instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 11, 2025
Use strings.Cut instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 11, 2025
Use strings.Cut instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 11, 2025
Use strings.Cut instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 11, 2025
Use strings.Index instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 11, 2025
Use strings.Index instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 12, 2025
Use strings.LastIndex instead of strings.Split to reduce memory allocations.

See #3403 for details.

```
goos: linux
goarch: amd64
pkg: github.com/zalando/skipper/net
cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
                                  │    HEAD~1    │                HEAD                 │
                                  │    sec/op    │   sec/op     vs base                │
RemoteHostFromLast/no_header-8      144.10n ± 7%   99.73n ± 6%  -30.79% (p=0.000 n=10)
RemoteHostFromLast/single_value-8    210.8n ± 5%   163.6n ± 5%  -22.39% (p=0.000 n=10)
RemoteHostFromLast/many_values-8     374.3n ± 4%   166.0n ± 3%  -55.66% (p=0.000 n=10)
geomean                              224.9n        139.4n       -38.01%

                                  │   HEAD~1    │                HEAD                │
                                  │    B/op     │    B/op     vs base                │
RemoteHostFromLast/no_header-8       64.00 ± 0%   48.00 ± 0%  -25.00% (p=0.000 n=10)
RemoteHostFromLast/single_value-8    64.00 ± 0%   48.00 ± 0%  -25.00% (p=0.000 n=10)
RemoteHostFromLast/many_values-8    224.00 ± 0%   48.00 ± 0%  -78.57% (p=0.000 n=10)
geomean                              97.17        48.00       -50.60%

                                  │   HEAD~1   │                HEAD                │
                                  │ allocs/op  │ allocs/op   vs base                │
RemoteHostFromLast/no_header-8      3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
RemoteHostFromLast/single_value-8   3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
RemoteHostFromLast/many_values-8    3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
geomean                             3.000        2.000       -33.33%
```

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
MustafaSaber pushed a commit that referenced this pull request Feb 12, 2025
* net: update BenchmarkRemoteHostFromLast

* update benchmark to use X-Forwarded-For value and report allocations.
* add more test cases

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

* net: use strings.LastIndex to parse header

Use strings.LastIndex instead of strings.Split to reduce memory allocations.

See #3403 for details.

```
goos: linux
goarch: amd64
pkg: github.com/zalando/skipper/net
cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
                                  │    HEAD~1    │                HEAD                 │
                                  │    sec/op    │   sec/op     vs base                │
RemoteHostFromLast/no_header-8      144.10n ± 7%   99.73n ± 6%  -30.79% (p=0.000 n=10)
RemoteHostFromLast/single_value-8    210.8n ± 5%   163.6n ± 5%  -22.39% (p=0.000 n=10)
RemoteHostFromLast/many_values-8     374.3n ± 4%   166.0n ± 3%  -55.66% (p=0.000 n=10)
geomean                              224.9n        139.4n       -38.01%

                                  │   HEAD~1    │                HEAD                │
                                  │    B/op     │    B/op     vs base                │
RemoteHostFromLast/no_header-8       64.00 ± 0%   48.00 ± 0%  -25.00% (p=0.000 n=10)
RemoteHostFromLast/single_value-8    64.00 ± 0%   48.00 ± 0%  -25.00% (p=0.000 n=10)
RemoteHostFromLast/many_values-8    224.00 ± 0%   48.00 ± 0%  -78.57% (p=0.000 n=10)
geomean                              97.17        48.00       -50.60%

                                  │   HEAD~1   │                HEAD                │
                                  │ allocs/op  │ allocs/op   vs base                │
RemoteHostFromLast/no_header-8      3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
RemoteHostFromLast/single_value-8   3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
RemoteHostFromLast/many_values-8    3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
geomean                             3.000        2.000       -33.33%
```

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

---------

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 13, 2025
Use strings.Index instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 13, 2025
Use strings.Index instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit that referenced this pull request Feb 14, 2025
Use strings.Index instead of strings.Split to reduce memory allocations.

See #3403 for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants