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

Limit the memory allocation when parsing ber encoded components in the LiteralSubject field #6782

Closed

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Feb 21, 2024

The go-ldap and asn-ber libraries both limit the message length in their fuzz tests because the default limit is 2GiB which crashes their test runners:

We can set the same limit before invoking ParseDN to avoid 2GiB memory being accidentally or maliciously allocated in the webhook component every time a LiteralSubject field value gets parsed.

I prefer this small change to the proposed re-implementation of ParseDN in #6761 because it allows us to continue using go-ldap library which I predict will do a better job than us of parsing these Subject strings which are compatible with a wide range of LDAP systems.

xref:

/kind bug

Mitigate a potential DoS against the webhook component, by limiting the maximum length of `ber` encoded literal subject field to 64KiB. Only affects those using the LiteralSubject feature.

@jetstack-bot jetstack-bot added kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 21, 2024
@wallrj
Copy link
Member Author

wallrj commented Feb 21, 2024

/retest

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 21, 2024
@wallrj wallrj changed the title Limit the memory allocation when parsing ber encoded components in distinguished name Limit the memory allocation when parsing ber encoded components in the LiteralSubject field Feb 21, 2024
Copy link
Member

@inteon inteon left a comment

Choose a reason for hiding this comment

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

@wallrj did you fuzz test this approach?

@wallrj
Copy link
Member Author

wallrj commented Feb 22, 2024

@wallrj did you fuzz test this approach?

I have not. It's a good idea though...I'm just not too familiar with the process. I see that @AdamKorcz added cert-manager project to the Google oss-fuzz project so perhaps there's a way to trigger that system for every PR?

I did link to the fuzz tests in the go-ldap and go-asn1-ber libraries in the PR description.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2024
Signed-off-by: Richard Wall <richard.wall@venafi.com>
Signed-off-by: Richard Wall <richard.wall@venafi.com>
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj changed the base branch from master to release-1.14 March 8, 2024 10:27
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2024
@wallrj wallrj force-pushed the limit-literalsubject-memory branch from c3b690d to 3d7ffdf Compare March 8, 2024 10:37
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from wallrj. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2024
@jetstack-bot
Copy link
Contributor

@wallrj: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cert-manager-release-1.14-e2e-v1-29-upgrade 3d7ffdf link true /test pull-cert-manager-release-1.14-e2e-v1-29-upgrade

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024
@jetstack-bot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@inteon
Copy link
Member

inteon commented Apr 15, 2024

This will be fixed by go-asn1-ber/asn1-ber#42

@inteon
Copy link
Member

inteon commented May 15, 2024

Fixed in the PRs listed in #7030.

@inteon inteon closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants