Skip to content

Java: add a few summary models - #15767

Merged
owen-mc merged 3 commits into
github:mainfrom
owen-mc:java/add-summary-models
Feb 29, 2024
Merged

Java: add a few summary models#15767
owen-mc merged 3 commits into
github:mainfrom
owen-mc:java/add-summary-models

Conversation

@owen-mc

@owen-mc owen-mc commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

The first commit adds summary models for a method that I noticed I had generated a bad summary model for in #14919. The second commit models a whole class (javax.crypto.Cipher), because the model generated in this PR created too much flow and caused the java/sensitive-log query on apache/geode to never terminate.

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Feb 29, 2024
@owen-mc
owen-mc requested a review from a team as a code owner February 29, 2024 12:36
@github-actions github-actions Bot added the Java label Feb 29, 2024
@github-actions

github-actions Bot commented Feb 29, 2024

Copy link
Copy Markdown
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.

Click to show differences in coverage

java

Generated file changes for java

  • Changes to framework-coverage-java.rst:
-    Java Standard Library,``java.*``,10,735,239,80,,9,,,25
+    Java Standard Library,``java.*``,10,737,239,80,,9,,,25
-    Totals,,308,18951,2558,336,16,128,33,1,409
+    Totals,,308,18953,2558,336,16,128,33,1,409
  • Changes to framework-coverage-java.csv:
- java.util,47,2,520,,,,,,,,,1,,,,,,,,,,,34,,,,2,,,,5,2,,1,2,,,,,,,,,,,,2,,,46,474
+ java.util,47,2,522,,,,,,,,,1,,,,,,,,,,,34,,,,2,,,,5,2,,1,2,,,,,,,,,,,,2,,,46,476

@atorralba atorralba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the reasoning behind SyntheticField[cipher.Encrypted]? It's a bit different than what I had in mind, so maybe my initial assessment wasn't accurate enough.

@owen-mc

owen-mc commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Hmm, I think I had the wrong idea about what was happening. I thought that if you had code like this:

Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher.init(Cipher.ENCRYPT_MODE, secretKey, ivParameterSpec);
byte[] encryptedPart1 = cipher.update(part1);
byte[] encryptedPart2 = cipher.update(part2);
byte[] finalPart = cipher.doFinal(part3);

then you could decrypt finalPart and get all of part1, part2 and part3 back, and that encryptedPart1 and encryptedPart2 weren't really needed. But on looking into it more I see that I was wrong. So maybe I don't need to have a field at all, and I just always model taint flow from the input to the output. (I don't think we need to keep track of it being encrypted or decrypted.)

@atorralba

atorralba commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

I just always model taint flow from the input to the output.

If we're not able to track encrypted/decrypted status, I think I'd prefer not propagating taint through those calls at all (i.e. make them neutrals). Because I can't think of many kinds of taint that would remain meaningful after an encryption operation, so this would add more FPs than TPs IMHO (where TPs are results that go both through encryption and decryption in the same path).

@owen-mc

owen-mc commented Feb 29, 2024

Copy link
Copy Markdown
Contributor Author

Okay - you are in a better position than I am to judge the trade-off. Shall I make wrap and unwrap neutral as well? They seem to be encrypting/decrypting a key.

@atorralba

Copy link
Copy Markdown
Contributor

Yes, I think the same logic applies.

Sorry that we agreed on this after your modeling work 😞.

@owen-mc
owen-mc merged commit 54031a8 into github:main Feb 29, 2024
@owen-mc
owen-mc deleted the java/add-summary-models branch February 29, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants