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

Do not hack certificate chain for Samsung certs #40

Closed
wants to merge 1 commit into from
Closed

Do not hack certificate chain for Samsung certs #40

wants to merge 1 commit into from

Conversation

salvogiangri
Copy link

No description provided.

@aviraxp
Copy link
Collaborator

aviraxp commented Aug 5, 2024

Can you elaborate it? Also maybe we can check for google public key?

@salvogiangri
Copy link
Author

Can you elaborate it? Also maybe we can check for google public key?

As part of the Knox suite, Samsung devices running One UI have additional keystore API's which allow attesting keys which are generated using Samsung's own certificate (SAK), this is mostly used on Samsung Knox powered apps and can be seen in action here: salvogiangri/SamsungKeyAttestation, vvb2060/KeyAttestation#15.
This commit prevents the service app to tamper the certificate chain if the key has been signed with SAK, as Samsung apps do also check if the certificate chain is valid, thus avoiding issues in the user end if they use Samsung services/apps which rely on it.

@aviraxp
Copy link
Collaborator

aviraxp commented Aug 5, 2024

Will that issue affect generating keypair mode? I am not sure how samsung api works actually. I think it won't generate a keypair for knox at all?

Also, I think we can just check for google public key and only conitinue if match, to prevent similar issues on other oem.

@privacyguy123
Copy link

privacyguy123 commented Aug 5, 2024

Considering I already have ALL apps added to target (for testing purposes mostly) on my S20 FE stock, what do we gain from not messing with Samsung apps certs? KnoxPatch allows the apps to open fine already?

@salvogiangri
Copy link
Author

Will that issue affect generating keypair mode? I am not sure how samsung api works actually. I think it won't generate a keypair for knox at all?

Samsung KeyStore userspace API's only differ from AOSP by the fact that they pass additional key tags to the KeyMaster/KeyMint TA (vvb2060/KeyAttestation#15 (comment)), which will then use the correct certificate/signing key accordingly or, if asked, add an additional extension for Knox-specific values such as warranty bit, trust boot, or PROCA.

Also, I think we can just check for google public key and only conitinue if match, to prevent similar issues on other oem.

I can check for the Google root public key instead, but it would be great for someone to confirm this won't affect devices with broken TEE.

@salvogiangri
Copy link
Author

Considering I already have ALL apps added to target (for testing purposes mostly) on my S20 FE stock, what do we gain from not messing with Samsung apps certs? KnoxPatch allows the apps to open fine already?

As I said earlier, Samsung apps will check if the certificate chain is valid and signed via SAK other than checking for ROT/Integrity Status. KnoxPatch only bypasses the ICD status verification in the TA (which blocks the key generation) by always forcing to add the integrity status extension in the generated key (hook code)

@privacyguy123
Copy link

privacyguy123 commented Aug 5, 2024

Considering I already have ALL apps added to target (for testing purposes mostly) on my S20 FE stock, what do we gain from not messing with Samsung apps certs? KnoxPatch allows the apps to open fine already?

As I said earlier, Samsung apps will check if the certificate chain is valid and signed via SAK other than checking for ROT/Integrity Status. KnoxPatch only bypasses the ICD status verification in the TA (which blocks the key generation) by always forcing to add the integrity status extension in the generated key (hook code)

I understand all this yet don't understand the use case for the commit. We need some sort of Samsung TV to see what it fixes in action?

@salvogiangri salvogiangri closed this by deleting the head repository Aug 17, 2024
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

Successfully merging this pull request may close these issues.

3 participants