Skip to content

Java: Refactor Cleartext Storage queries #6493

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 23, 2021
Merged

Java: Refactor Cleartext Storage queries #6493

merged 5 commits into from
Sep 23, 2021

Conversation

atorralba
Copy link
Contributor

@atorralba atorralba commented Aug 17, 2021 •
edited
Loading

This PR splits the SensitiveStorage library in several files, one for each query. The *Query.qll naming convention has been followed, and the base CleartextStorageQuery.qll library has been generalized to have extensible sinks, sanitizers and taint steps. The encryption sanitizer that was being used in the Shared Preferences query has also been generalized, and now all sensitive storage queries benefit from it.

@atorralba atorralba requested a review from a team as a code owner August 17, 2021 10:12
@github-actions
Copy link
Contributor

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

@atorralba atorralba changed the title Java: Refactor Sensitive Storage queries Java: Refactor Cleartext Storage queries Aug 17, 2021
@atorralba atorralba force-pushed the atorralba/cleartext-storage-query-refactor branch 2 times, most recently from 4dbd67f to 710a9f3 Compare August 23, 2021 14:56
@github-actions
Copy link
Contributor

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The differences can be found in the comparison artifact of this workflow run.

@atorralba atorralba mentioned this pull request Aug 31, 2021
Merged
@atorralba atorralba force-pushed the atorralba/cleartext-storage-query-refactor branch from 710a9f3 to d590906 Compare September 3, 2021 11:26
@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2021

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. A recent commit removed the previously reported differences.

@atorralba
Copy link
Contributor Author

Force pushed to rebase this refactor to main instead of building on top of previous PRs. This should facilitate the review.

@atorralba atorralba added the no-change-note-required This PR does not need a change note label Sep 3, 2021
This was referenced Sep 3, 2021
Merged
Merged
@aschackmull
Copy link
Contributor

This definitely changes the performance characteristics of these 3 queries, and it no longer makes sense for SensitiveSource.flowsToCached to be cached. This may be perfectly fine, but you should make sure to do a thorough performance evaluation.

@atorralba
Copy link
Contributor Author

atorralba commented Sep 22, 2021 •
edited
Loading

@aschackmull performance evaluation done. Apparently there's no negative impact on performance after this PR (CleartextStorageProperties.ql took +164s but CleartextStorageCookie.ql took -223s, so it seems like a normal tradeoff depending on which query runs first, plus a ~60s improvement).

Still not sure why the integration tests are failing though.

@aschackmull
Copy link
Contributor

Do you have a link to the perf eval?

@atorralba atorralba force-pushed the atorralba/cleartext-storage-query-refactor branch from b12a0b7 to d0b9920 Compare September 23, 2021 08:42
@atorralba @aschackmull
Apply code review comments
b52a2cd 
Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
@atorralba
Copy link
Contributor Author

Thanks for your comments @aschackmull, applied in b52a2cd.

@aschackmull aschackmull merged commit a031b2a into github:main Sep 23, 2021
@atorralba atorralba deleted the atorralba/cleartext-storage-query-refactor branch September 23, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschackmull aschackmull aschackmull approved these changes

Assignees
No one assigned
Labels
documentation Java no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@atorralba @aschackmull