Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

security issues with URP logic #887

Closed
diracdeltas opened this issue Feb 11, 2019 · 3 comments
Closed

security issues with URP logic #887

diracdeltas opened this issue Feb 11, 2019 · 3 comments

Comments

@diracdeltas
Copy link
Member

cc @bbondy @jumde

there are 2 security improvements we should make to the URP logic:

  1. in

    service.fetchCustomHeaders() { headers, error in
    , custom headers fetched from the server should only be accepted if the header name is X-Brave-Partner; otherwise log/throw an error. This prevents an attacker who controls the Brave endpoint from being able to set any headers other than X-Brave-Partner.

  2. for Coinbase, which uses cookies instead of headers, you should do some additional validation before inserting the cookie(s) into the cookie store (https://github.com/brave/brave-ios/pull/823/files#diff-60f6d7911cf19ceb7bcfcde55f9bc740R240): (a) validate that the cookie is being inserted only for coinbase.com, otherwise throw an error; (b) validate that the cookie name is __Secure-X-Brave-Partner.

@diracdeltas
Copy link
Member Author

On second thought, re: cookies - why fetch them from the server at all?

The original reason for fetching headers from the server was so that the promos could be changed without a browser update; but in the case of cookies, a browser update is needed anyway for updating this list: https://github.com/brave/brave-ios/pull/823/files#diff-60f6d7911cf19ceb7bcfcde55f9bc740R15

@jhreis
Copy link
Contributor

jhreis commented Feb 11, 2019

Currently the list is a blacklist for partners that do not want the HTTP Header (not for cookie opt-in). This was a short-term patch, and @aekeus was looking at adding additional data to the request to specify which solution (or both) each partner would use.

We just didn't have time, given the intense constraints we had.

@diracdeltas
Copy link
Member Author

TODO: make sure header value cannot be maliciously formed to create a new header name

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants