-
Notifications
You must be signed in to change notification settings - Fork 983
Use a random boundary value by default #625
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 a random boundary value by default #625
Conversation
| * deliberate choice. | ||
| * </p> | ||
| * <p> | ||
| * IMPORTANT: it is responsibility of the caller to validate / sanitize content of body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benweissmann Restore this section. It says IMPORTANT for a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done -- i had added a similar IMPORTANT note in setBoundary to note the considerations specifically around boundary values, but happy to leave it here as well.
| * Generates a random boundary using UUID. The UUID is a v4 random UUID generated from a cryptographically-secure | ||
| * random source. | ||
| * <p> | ||
| * A cryptographically-secure random number source is used to avoid security issues similar to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benweissmann Remove references to this CVE. It has nothing to do with the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think of the revised note -- I've removed the reference to the CVE, but I think it's important to note the source (and randomness guarantees) of the UUID for two reasons:
- As you've discussed, it's the responsibility of the caller to enforce any security model for their application, so we should document the source of the value here so they can make those decisions.
- So future contributors know that the choice of randomness here was intentional, and to consider that if there's any future changes to the default boundary value selection
ok2c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benweissmann That sounds reasonable.
|
Hey folks, just wanted to check if the plan is to merge this soon? Before the next release? |
See more context in discussion from #619.
The advantage of using a random boundary by default are:
Of note, the current default behavior makes this attack much, much simpler than the one in that CVE, because an attacker does not need to guess the random value from an insecure random number generator; all existing libraries and applications that use httpcomponents-client will start using the new hard-coded default value when they upgrade unless they explicitly opt-in to the new random behavior or were already generating their own boundary values.
As a practical, in-the-wild example of this, consider this Modern Treasury Kotlin library function, which uses uses our MultipartFormBuilder API to hit the Create Document api endpoint.
Consider the most common likely usage of this function:
A Kotlin web application allows users to upload documents, like I-9 forms or other financial or compliance paperwork
The server-side component of this application receives the document from an authenticated user, and then uses this API method to attach the user-provided document to corresponding Modern Treasury entity. For example, it might take an employee's I-9 (employment verification form) and attach it to that employee's bank account record in a payroll system.
A malicious user could upload a PDF that ends with
Now, instead of the server-side component of this application sending the user-provided PDF and the server-provided "documentable_id", it would be tricked into sending an attacker-controlled documentable_id, which would associate the financial or compliance paperwork with the wrong account.
While there is disagreement from some maintainers on some of these points, I would contend that any one of these five arguments should be sufficient to use the secure-random UUID default behavior, rather than the currently-implemented fixed-value default (or, for that matter, the currently-released insecure random default).