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

Scrubbing bug causing PII leaks in Rollbar PHP library #463

Open
ArturMoczulski opened this issue Jun 24, 2019 · 1 comment
Open

Scrubbing bug causing PII leaks in Rollbar PHP library #463

ArturMoczulski opened this issue Jun 24, 2019 · 1 comment

Comments

@ArturMoczulski
Copy link
Contributor

ArturMoczulski commented Jun 24, 2019

Feedback from one of the users:

While investigating the leak of some PII information from our systems to Rollbar I identified a pretty serious bug in your PHP library (https://github.com/rollbar/rollbar-php).

Because of how the scrubbing feature is implemented, form submissions that cause exceptions and subsequently trigger Rollbar reporting can be excluded from the usual scrubbing process under certain circumstances.

Technical explanation:
In src/Scrubber.php on line 81-82 you are serializing in this case the body of a request containing form fields into an array. Usually this is no problem. However, when you have form arrays and they appear out of order it can cause line 81 to rearrange the hierarchy of the request (because of the array html fields), which causes the line 82 condition to fail which then means scrubbing is skipped on that field.

Scrubbing works:
login[username]=test@test.com&login[password]=secret

Scrubbing fails:
login[username]=test@test.com&unrelatedField=123&login[password]=secret

When scrubbing fails, passwords and other sensitive PII information are sent to Rollbar exempt from the usual scrubbing process.

We're implementing mitigations on our end to prevent this happening in the short term but is there any chance this bug could be fixed?

@ArturMoczulski ArturMoczulski self-assigned this Jun 24, 2019
@bishopb
Copy link
Contributor

bishopb commented Nov 6, 2020

Perhaps a more robust approach would be to scrub everything except those identified in an allow list. That mode could be engaged in a backward compatible way with a new flag at configuration time, along the lines of scrubAllExcept: [ 'login' ]. Thoughts?

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

No branches or pull requests

2 participants