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

Check XBE signature on Open #1523

Merged
merged 3 commits into from Jan 8, 2019

Conversation

Projects
None yet
4 participants
@GXTX
Copy link
Contributor

GXTX commented Jan 6, 2019

Check XBE signature on open & allow users to disable this pop-up using the setting allowAdminPrivilege.

I think this will produce better overall compatibility reports of games as it incentivizes users to use unmodified games.

GXTX added some commits Jan 6, 2019

Show resolved Hide resolved src/gui/WndMain.cpp Outdated
@JayFoxRox

This comment has been minimized.

Copy link

JayFoxRox commented Jan 6, 2019

While a good idea on paper, this feels like a bad idea in practice.

At least CheckXbeSignature should check all valid / common signatures: MS retail, MS debug, MS chihiro.
I believe some Xbox Live betas also had custom signatures.

Currently, the warning is a no-op and gives a false sense of security. Because it allows zero-signatures from homebrew, which can be malicious, because it wasn't verified by a trusted source.
I believe there's hardly any XBE which does not succeed the check in the current form, and if it does, it's probably not malicious, but just corrupted.
Meanwhile, even malicious XBEs are allowed.
(I was wrong: This is not the case. The signature checks for homebrew XBEs will always fail)

So the homebrew signatures, like habibi or a zero signature, would have to be allowed, but warned.
(This should be happening already)

The possible main issue: I'm also not sure about the signature specifics, but if it's also checking header fields like "allowed-media-flags", then almost any XBE in existence will now be reported (affecting usability, and scaring people away). Many dumping solutions will modify these flags. To my knowledge, some tools also patched code to work around security checks.

Delete x_Xbe if it isn't needed after checking signature
This also changes the order of operations for the sigchk if statement.
@GXTX

This comment has been minimized.

Copy link
Contributor

GXTX commented Jan 7, 2019

Went ahead and fixed your concerns. As to the last paragraph there, like we chatted about sometime last night the biggest reason for this PR is users are currently for the most part using non-original versions of games and I believe this pollutes the compatibility tracker immensely by not only having different versions of games but also different flags for each game. There also comes a risk in using these modified versions such as if a bad actor were to inject malicious code after a user uses closed source tools to create personal copies.

If the user doesn't know how to set these tools not to modify game code I think that's a separate issue we need to address.

@RadWolfie

This comment has been minimized.

Copy link
Member

RadWolfie commented Jan 8, 2019

CheckXbeSignature is using XePublicKeyData which is already provided in the source code. I'm not sure if debug kit (most likely yes) and chihiro are using the same public key. If not, then someone will have to get the RSA public key from the hardware.

I believe some Xbox Live betas also had custom signatures.

I'm not sure about this since I don't have access to any of Xbox Live games. Plus it should produce a failure on real hardware if it was using a different RSA public key.

@RadWolfie

This comment has been minimized.

Copy link
Member

RadWolfie commented Jan 8, 2019

Other than that, looks good to me since it will popup only on GUI's end when open an xbe file.

@LukeUsher

This comment has been minimized.

Copy link
Member

LukeUsher commented Jan 8, 2019

Thanks!

Currently this shows for Chihiro executables, but that's because we don't have the Chihiro xbe key in Cxbx-R yet. I'll address that in a later commit.

@LukeUsher LukeUsher merged commit c24f83f into Cxbx-Reloaded:develop Jan 8, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment