[x] This is not a duplicate of an existing merge request.
[x] I believe this falls into the scope of the project and should be part of the built-in functionality.
[x] My code follows the code style of this project.
[x] I have added tests to cover my changes, wherever they are necessary. (I didn't find any tests for password hashing, so I don't think it's relevant to add a test for checking hashes)
[x] All new and existing tests pass.
Changes
Changed functionality
This PR simply changes how the IPC password provided matches the one in the config. Previously, the provided password would be hashed, converted to base64, and then string-compared to the hash in the config. The problem with using a string comparison is that how long the comparison takes depends on how much of the strings match at the beginning. This creates a possible side-channel attack for live-decoding the password. For a password hashed with an attacker-unknown salt, this shouldn't be a problem, but in the case where it's hashed with the default salt or is not hashed at all, then this could leak information about the hash (or plaintext password when unhashed). Actually exploiting this would most likely be impractical (any attack over a network would likely have more network jitter than actually meaningful delay), but I think it's worth addressing.
This PR moves the actual comparison from ApiAuthenticationMiddleware.cs to ArchiCryptoHelper.cs, performs the comparison in raw bytes (converting the base64 hash back to binary), and uses CryptographicOperations.FixedTimeEquals to ensure the comparison doesn't leak any information about the IPC password (other than the length if the password is unhashed, but there's not really a solution for that). This should improve the security of the IPC password.
Additional info
Moving the actual hash comparison was done largely for encapsulation. Checking if a password matches a given hash is arguably a "Crypto" operation, so providing a method for it in ArchiCryptoHelper helps to keep all the crypto operations in the same source file. This would also make it easier to support changes to how hashing is handled, such as adding support for the "modular crypt format" (i.e. combining password hash and salt into one string).
I did not change how the hash is tested for Parental Codes, because in that case, ASF itself is trying passwords. Since there isn't an opportunity for an external attacker, it's less important to make this constant-time. I could also make this constant-time, but it would make ArchiCryptoHelper.RecoverSteamParentalCode slower for minimal benefit, so I don't think it's worth it.
Checklist
Changes
Changed functionality
This PR simply changes how the IPC password provided matches the one in the config. Previously, the provided password would be hashed, converted to base64, and then string-compared to the hash in the config. The problem with using a string comparison is that how long the comparison takes depends on how much of the strings match at the beginning. This creates a possible side-channel attack for live-decoding the password. For a password hashed with an attacker-unknown salt, this shouldn't be a problem, but in the case where it's hashed with the default salt or is not hashed at all, then this could leak information about the hash (or plaintext password when unhashed). Actually exploiting this would most likely be impractical (any attack over a network would likely have more network jitter than actually meaningful delay), but I think it's worth addressing.
This PR moves the actual comparison from
ApiAuthenticationMiddleware.cstoArchiCryptoHelper.cs, performs the comparison in raw bytes (converting the base64 hash back to binary), and usesCryptographicOperations.FixedTimeEqualsto ensure the comparison doesn't leak any information about the IPC password (other than the length if the password is unhashed, but there's not really a solution for that). This should improve the security of the IPC password.Additional info
Moving the actual hash comparison was done largely for encapsulation. Checking if a password matches a given hash is arguably a "Crypto" operation, so providing a method for it in
ArchiCryptoHelperhelps to keep all the crypto operations in the same source file. This would also make it easier to support changes to how hashing is handled, such as adding support for the "modular crypt format" (i.e. combining password hash and salt into one string).I did not change how the hash is tested for Parental Codes, because in that case, ASF itself is trying passwords. Since there isn't an opportunity for an external attacker, it's less important to make this constant-time. I could also make this constant-time, but it would make
ArchiCryptoHelper.RecoverSteamParentalCodeslower for minimal benefit, so I don't think it's worth it.