{"action":"created","issue":{"url":"https://api.github.com/repos/defuse/php-encryption/issues/240","repository_url":"https://api.github.com/repos/defuse/php-encryption","labels_url":"https://api.github.com/repos/defuse/php-encryption/issues/240/labels{/name}","comments_url":"https://api.github.com/repos/defuse/php-encryption/issues/240/comments","events_url":"https://api.github.com/repos/defuse/php-encryption/issues/240/events","html_url":"https://github.com/defuse/php-encryption/issues/240","id":150260135,"number":240,"title":"Fix the Key Replacement Attack","user":{"login":"defuse","id":4211072,"avatar_url":"https://avatars.githubusercontent.com/u/4211072?v=3","gravatar_id":"","url":"https://api.github.com/users/defuse","html_url":"https://github.com/defuse","followers_url":"https://api.github.com/users/defuse/followers","following_url":"https://api.github.com/users/defuse/following{/other_user}","gists_url":"https://api.github.com/users/defuse/gists{/gist_id}","starred_url":"https://api.github.com/users/defuse/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/defuse/subscriptions","organizations_url":"https://api.github.com/users/defuse/orgs","repos_url":"https://api.github.com/users/defuse/repos","events_url":"https://api.github.com/users/defuse/events{/privacy}","received_events_url":"https://api.github.com/users/defuse/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/defuse/php-encryption/labels/SECURITY","name":"SECURITY","color":"e11d21"},{"url":"https://api.github.com/repos/defuse/php-encryption/labels/v2.0","name":"v2.0","color":"006b75"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/defuse/php-encryption/milestones/3","html_url":"https://github.com/defuse/php-encryption/milestones/v2.0%20release","labels_url":"https://api.github.com/repos/defuse/php-encryption/milestones/3/labels","id":1686201,"number":3,"title":"v2.0 release","description":"","creator":{"login":"defuse","id":4211072,"avatar_url":"https://avatars.githubusercontent.com/u/4211072?v=3","gravatar_id":"","url":"https://api.github.com/users/defuse","html_url":"https://github.com/defuse","followers_url":"https://api.github.com/users/defuse/followers","following_url":"https://api.github.com/users/defuse/following{/other_user}","gists_url":"https://api.github.com/users/defuse/gists{/gist_id}","starred_url":"https://api.github.com/users/defuse/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/defuse/subscriptions","organizations_url":"https://api.github.com/users/defuse/orgs","repos_url":"https://api.github.com/users/defuse/repos","events_url":"https://api.github.com/users/defuse/events{/privacy}","received_events_url":"https://api.github.com/users/defuse/received_events","type":"User","site_admin":false},"open_issues":6,"closed_issues":3,"state":"open","created_at":"2016-04-06T01:09:20Z","updated_at":"2016-04-22T04:12:51Z","due_on":"2016-05-01T06:00:00Z","closed_at":null},"comments":5,"created_at":"2016-04-22T04:06:43Z","updated_at":"2016-04-22T20:11:49Z","closed_at":null,"body":"I found a protocol interaction attack (aka a [chosen-protocol attack](https://www.schneier.com/cryptography/archives/1997/01/protocol_interaction.html))!\r\n\r\nConsider the following two scenarios.\r\n\r\n**Scenario A:** A web server is configured with a password P<sub>1</sub>. That web server uses this library's `Crypto::encryptWithPassword()` to encrypt arbitrary user-uploaded strings and returns the ciphertext to the user.\r\n\r\n**Scenario B:** A web server is configured with a password P<sub>2</sub>. That web server allows the user to upload a serialization of a `KeyProtectedByPassword`. It attempts to unlock the `KeyProtectedByPassword` with P<sub>2</sub> and if successful encrypts its bitcoin wallet private keys with the resulting `Key` and broadcasts the ciphertext to the world.\r\n\r\nThese are obviously contrived, but bear with me.\r\n\r\nIndividually, both of these scenarios are secure (but useless) on their own. In Scenario A the server is just a chosen plaintext oracle, only the server with P<sub>1</sub> could decrypt the ciphertexts it creates. In Scenario B, what the server is doing should be fine, since the IND-CCA2 property guarantees that it shouldn't be practical to find a valid `KeyProtectedByPassword` for P<sub>2</sub> and so its bitcoins are safe.\r\n\r\nHowever, if Scenario A and Scenario B are happening at the same time, and P<sub>1</sub> = P<sub>2</sub> then the combined system is *not* secure.\r\n\r\nLet P = P<sub>1</sub> = P<sub>2</sub>. The attacker can first compute the `saveToAsciiSafeString()` string representation of the all-zero-byte key. They provide that string to the server in Scenario A, obtaining the encryption of it under P. Now they can provide that ciphertext to the server in Scenario B, which will unlock it to the all-zero-byte-key using P and broadcast their bitcoin private keys encrypted under the all-zero-byte-key, which the attacker can easily decrypt.\r\n\r\n(Edit to clarify: The two servers can trust each other, so they can use the same password. The server in Scenario B might trust the server in Scenario A not to steal its bitcoins. The point is, the attacker can do this *without ever learning the password* and *without ever requiring either of the servers to behave maliciously*.)\r\n\r\nIn other words, if the same password is re-used in two different protocols, one providing the attacker with a chosen-plaintext oracle for password encryption, we lose the integrity guarantees for any `KeyProtectedByPassword` that are protected with the same password.\r\n\r\nI found this bug while writing documentation. I was making this change to unlockKey when I realized it was possible: https://github.com/defuse/php-encryption/pull/226/commits/f6a839e38ad87a019ff160892e152a7a8b80c998\r\n\r\nWhy did this vulnerability get introduced? Because when I added `KeyProtectedByPassword` I did not explicitly realize I was *adding a second protocol* which could interact with the user's application's protocol. \r\n\r\nIs this really a vulnerability? If the user wants to use this library with the same secret in two different protocols then it *is* their responsibility to ensure there are no interactions between them. However, it would not have been possible to tell that this kind of interaction exists from our API and documentation alone. This kind of interaction could have been avoided in principle, and the user reading our API and docs might have assumed it was. So it is a vulnerability.\r\n\r\nBonus points if you can come up with more-realistic and less-contrived examples for Scenario A and Scenario B."},"comment":{"url":"https://api.github.com/repos/defuse/php-encryption/issues/comments/213574021","html_url":"https://github.com/defuse/php-encryption/issues/240#issuecomment-213574021","issue_url":"https://api.github.com/repos/defuse/php-encryption/issues/240","id":213574021,"user":{"login":"defuse","id":4211072,"avatar_url":"https://avatars.githubusercontent.com/u/4211072?v=3","gravatar_id":"","url":"https://api.github.com/users/defuse","html_url":"https://github.com/defuse","followers_url":"https://api.github.com/users/defuse/followers","following_url":"https://api.github.com/users/defuse/following{/other_user}","gists_url":"https://api.github.com/users/defuse/gists{/gist_id}","starred_url":"https://api.github.com/users/defuse/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/defuse/subscriptions","organizations_url":"https://api.github.com/users/defuse/orgs","repos_url":"https://api.github.com/users/defuse/repos","events_url":"https://api.github.com/users/defuse/events{/privacy}","received_events_url":"https://api.github.com/users/defuse/received_events","type":"User","site_admin":false},"created_at":"2016-04-22T20:11:49Z","updated_at":"2016-04-22T20:11:49Z","body":"I don't think we should add a domain separation parameter because that would be too much of a change and we *need* to get v2.0.0 reviewed and out soon. How about this: Inside `KeyProtectedByPassword`, instead of just passing the user's password on, we compute SHA256(password) and use that as the password to encrypt the key. I like it because:\r\n\r\n1. It's easy to reason about (clearly it isn't any *less* secure than before).\r\n2. It provides enough domain separation. The only way a `KeyProtectedByPassword` password could be equivalent to a `Crypto` password would be if you found a SHA256 fixed point, or you found an equivalent key for a deeper layer of the crypto."}}