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

Use SecureRandom instead of Random #1133

Merged
merged 1 commit into from
Mar 25, 2021
Merged

Conversation

marci4
Copy link
Collaborator

@marci4 marci4 commented Mar 22, 2021

Description

Related Issue

Fixes #1132

How Has This Been Tested?

Connect to a remote server.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@PhilipRoman
Copy link
Collaborator

I've looked into this a bit, and I no longer think there is any direct vulnerability here. The cache poisoning exploit requires that there is malicious code controlling the websocket client. Since this is a Java library, any code controlling this client is typically already running with full Java privileges and can do whatever it wants on the user's computer.

There might be a situation though, where a user of this library sends unobscured data (without using Json or base64) obtained from a malicious source (like someone implementing a web browser using this library, which is in practice very unlikely).

Either way, this PR should be merged, just to follow the advice in the spec, but as far as I understand, the masking mechanism is primarily for web browsers.

@marci4 marci4 merged commit d0f5731 into TooTallNate:master Mar 25, 2021
@marci4 marci4 deleted the Issue1132 branch March 25, 2021 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draft_6455 flagged by Veracode CWE-331 replace Random with SecureRandom
2 participants