You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Crispy-fried-chicken (via GitHub)" <gi...@apache.org> on 2023/08/24 04:18:37 UTC
[GitHub] [pulsar] Crispy-fried-chicken opened a new pull request, #21061: [fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals
Crispy-fried-chicken opened a new pull request, #21061:
URL: https://github.com/apache/pulsar/pull/21061
<!--
### Contribution Checklist
- PR title format should be *[fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #21053
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
I may have discovered a method in the newest version of [pulsar](https://github.com/apache/pulsar), which has "Observable Timing Discrepancy" vulnerability. The vulnerability is located in the method org.apache.pulsar.broker.authentication.SaslRoleTokenSigner.verifyAndExtract(String signedStr) . The vulnerability bears similarities to a recent CVE disclosure https://github.com/advisories/GHSA-54g4-5cf6-hjp3 in the "apache/hive" project.
The source vulnerability information is as follows:
> **Vulnerability Detail:**
>
> **CVE Identifier:** CVE-2020-1926
>
> **Description**:Apache Hive cookie signature verification used a non constant time comparison which is known to be vulnerable to timing attacks. This could allow recovery of another users cookie signature. The issue was addressed in Apache Hive 2.3.8.
>
> **Reference:**https://nvd.nist.gov/vuln/detail/CVE-2020-1926
>
> **Patch**: https://github.com/apache/hive/commit/ee5a6be81a87bb21b3779edad6e61b67b365997b
**Vulnerability Description**: The vulnerability exists within the String verifyAndExtract(String signedStr) method of the org.apache.pulsar.broker.authentication.SaslRoleTokenSigner class. It is responsible for authenticating and extracting SaslRoleToken. SaslRoleToken is a token used for authentication and authorization, which contains a signed string of information about roles and permissions.**But the authentication snippet is similar to the vulnerable snippet for CVE-2020-1926 and may have the same consequence as CVE-2020-1926, where isvulnerable to timing attacks.** Therefore, maybe you need to fix the vulnerability with much the same fix code as the CVE-2020-1926 patch.
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
I fix it by replacing `String.equals` with `MessageDigest.isEqual`
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
**This change is a trivial rework / code cleanup without any test coverage.**
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #21061: [fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21061:
URL: https://github.com/apache/pulsar/pull/21061#issuecomment-1690973068
@Crispy-fried-chicken Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall merged pull request #21061: [fix][broker] Use MessageDigest.isEqual when comparing digests
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall merged PR #21061:
URL: https://github.com/apache/pulsar/pull/21061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Crispy-fried-chicken commented on pull request #21061: [fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals
Posted by "Crispy-fried-chicken (via GitHub)" <gi...@apache.org>.
Crispy-fried-chicken commented on PR #21061:
URL: https://github.com/apache/pulsar/pull/21061#issuecomment-1691676634
> Thanks for your PR. Looks good to me.
I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #21061: [fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on PR #21061:
URL: https://github.com/apache/pulsar/pull/21061#issuecomment-1691678891
> I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?
@Crispy-fried-chicken Yes, you need to fix the code style issue:
```
Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,9] (whitespace) WhitespaceAround: 'if' is not followed by whitespace.
Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,63] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Crispy-fried-chicken commented on pull request #21061: [fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals
Posted by "Crispy-fried-chicken (via GitHub)" <gi...@apache.org>.
Crispy-fried-chicken commented on PR #21061:
URL: https://github.com/apache/pulsar/pull/21061#issuecomment-1691690330
> > I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?
>
> @Crispy-fried-chicken Yes, you need to fix the code style issue:
>
> ```
> Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,9] (whitespace) WhitespaceAround: 'if' is not followed by whitespace.
> Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,63] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
> ```
> > I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?
>
> @Crispy-fried-chicken Yes, you need to fix the code style issue:
>
> ```
> Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,9] (whitespace) WhitespaceAround: 'if' is not followed by whitespace.
> Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,63] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
> ```
OK, I've already fix it and push to my fork repo. How can I merge it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Crispy-fried-chicken commented on pull request #21061: [fix][broker]fix 'Observable Timing Discrepancy' vulnerability by replacing String.equals
Posted by "Crispy-fried-chicken (via GitHub)" <gi...@apache.org>.
Crispy-fried-chicken commented on PR #21061:
URL: https://github.com/apache/pulsar/pull/21061#issuecomment-1691693604
> > I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?I saw the Pulsar CI / Build and License check (pull_request) has been failed. Is there anything shall I change?
>
> @Crispy-fried-chicken Yes, you need to fix the code style issue:
>
> ```
> Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,9] (whitespace) WhitespaceAround: 'if' is not followed by whitespace.
> Error: src/main/java/org/apache/pulsar/broker/authentication/SaslRoleTokenSigner.java:[79,63] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
> ```
OK, I've already fix it. Please review it again? Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org