Constant time RSA PKCS#1 v1.5 depadding #2948
Conversation
There was a problem hiding this comment.
We can see that it is very hard to avoid time leakage throughout the whole software stack. It is not simply a matter of implementing sc_pkcs1_strip_02_padding_constant_time() and being done with it. Obviously, some files needed to be touched that seem unrelated at first glance. I have some doubts that we can keep up this effort without accidentally introducing new leakage.
That's why we should think again about the impact of deactivating RSA PKCS#1 v1.5 depadding in software. If the impact should be too heavy, then a legacy option could be introduced to enable this via opensc.conf. For now, this PR seems to solve the issue, so I think we can discuss this seperately without pressure.
|
I will try to draw my view on this PR. Let's assume that from the moment the token returns the APDU, OpenSC processes the APDU and returns the return value to the application in constant time. The attacker can find out from the timing attack on the application whether the decrypt operation was completed correctly or ended with an error. |
|
The matter is complicated as even a shortcut due to some error handling may lead to a side channel. If you want to dive into this, you may want to check out the marvin page and read the technical paper. I believe, the changes in this PR were guided by the toolkit for detecting time based side channels... |
|
As I suggested, even if we make the OpenSC code so robust that it still maintains a constant time during decryption (for both correct and incorrect resulting plaintext), the resulting processing in the application that called the pkcs11 opensc service will still be vulnerable to a side channel timing attack. Even if the pkcs1 type 2 padding is removed directly in the token, we can still determine from a side channel attack on the application whether the plaintext met / did not meet the pkcs1 type 2 conditions (because it is possible to determine from the timing whether the application is processing the decrypted result, or is going the other way - an error path). The only thing left is to consider how much we plan to complicate the OpenSC code, when a possible attacker calmly focuses on a timing attack that reveals the necessary information from the application calling opensc. |
|
We should avoid leak of information that allows the attacker to distinguish a padding error from any other error, because this allows to gain information about the plaintext. This is also an API related problem, because the timing of handling buffers of different lengths are different. That's why changes in the PKCS#11/Minidriver/CTK code are needed. Above that, the attacker will only know that decryption failed without getting information about the plaintext. It could be possible that I'm explaining this not well enough or even incorrectly. If you like, we can get you in touch with the author of the marvin paper. |
|
Yes, I understand that we mask absolutely everything... the attacker finds out that the decrypt was wrong, but he doesn't know if it was a mistake in the padding or another mistake. Although in my opinion, if the attacker finds out from the side channel that there was a decryption error, he can almost certainly rely on the fact that it was an error when removing the padding. You're right, we can't do more. It is OK. |
even if it's done in hardware, the error must be returned to the calling application in constant time
Correct, and applications like openssl should already be doing that; so, while it's hard, it's not impossible. For a library like opensc there are only two options: don't create a side-channel where there wasn't one, or don't support PKCS#1v1.5 decryption, anything else is a vulnerability in OpenSC. It won't matter if the calling code is processing the result in side-channel free way if the returned value already has the side-channel baked-in. |
xhanulik
force-pushed
the
constant-pkcs1
branch
from
d2c5c32
to
47d2340
Compare
|
The above comments should by fixed by now. |
|
There is error in this implementation for missing message - openssl correctly returns an empty file for an empty message, test script: MyEID card or OsEID simulator, padding removal turned off in opensc.conf: Using this PR ... |
|
Thanks for catching, there was a redundant check for a empty message in the depadding function. Added also the corresponding unit test. |
|
Thank you, at the moment the tests are going without any problems. Just for fun, I measured the approximate depadding time (average of 10 tests, i5-3470, Linux version 6.1.0-15-rt-amd64) - time from APDU response to C_finalize() , with PR/without this PR:
The truth is that the attacker has worse options for measuring this time, but thanks to other bugs in current processors, we have no way to prevent him from accessing this information. Anyway, I agree with the fact that we try to disable the possible attack as much as possible, and this PR contributes to that. |
|
@popovec Those results don't look right to me... Veronika has been running tests to verify that there are no differences in behaviour on the nanosecond level, not µs level. If, in your testing, you're running the same ciphertext over and over, and just averaging out the time over such repeated measurements, then you're not measuring the differences in the behaviour of the code, you're measuring the average frequency of the CPU that was performing the test. Please read the Out of the Box Testing paper linked from the Marvin page for details, and the marvin-toolkit for a practical example of applying conclusion from that paper. |
|
Let's describe once again the methodology I use: Here is an example for some measurements that I quickly made:
Yes, sometimes it is reversed, i.e. incorrect padding takes longer than correct padding, but it seems to me that it makes it possible to identify quite precisely which message has faulty padding.. |
|
I have measured the whole For generating the input plaintexts and performing the time analysis, I used marvin-toolkit (slightly adjusted to generate plaintexts instead of ciphertexts). The final report.txt looks ok. However, I can rerun the tests with the current state of PR to double-check. |
|
Your testing method seems perfectly fine to me. I looked through the entire PR and it also seems fine. I still don't know the reason why the measurement I made gives such a good indication of the nature of the padding. I will try to find out more precisely where the problem arises. The log shows which lines I use to measure the time difference: |
|
One more thing, pkcs11-tool tries to call C_Decrypt first, if it fails (it ends with an error) it tries to call C_DecryptUpdate (even if it is non-standard, it can also be used for asymmetric decryption), however, this ends in the case of incorrect padding as follows:
When I conducted my measurements, I was actually measuring part of C_DecryptUpdate and not C_Decrypt itself.
I don't dare to judge whether there is any correlation between padding and time, rather no than yes. |
|
hmm, isn't it actually incorrect to call Or the behaviour you're talking about is for handling a buggy PKCS#11 driver, that doesn't implement The measurements you've quoted are too variable to be able to say anything about presence or absence of a side-channel:
unless there's a consistent difference, then no, it can't be used like that |
When I took the measurements on the i5, the correlation of time and type of padding was strangely very accurate... I don't know why. It was probably a coincidence. However, the measurements on the Opteron are far less convincing. @xhanulik: please check if we don't have a condition in C_DecryptUpdate that should also be executed in constant time. |
|
Let me ask it the other way:
@xhanulik please correct me if you have the data, but based on the necessary changes I wouldn't expect the side-channel to be larger than 10 µs. With a processing time in the order of 500 µs, st.dev of 90µs, with just 10 measurements even Student t test is unlikely to have a statistically significant result with normally distributed data. So yes, very likely a coincidence. |
We can safely expect that in programming languages that do not have pkcs11 support in their libraries, anyone can call pkcs11-tool via execve() or a similar call. An example can be a bash script where someone uses the pkcs11-tool to decrypt some data. Even if the relevant programming language has pkcs11 support in it, someone can call pkcs11-tool as a decryption tool... Much more dangerous things happen on the internet, people download an unverified docker file and run it… |
|
src/libopensc/pkcs15-sec.c I was particularly interested in this place in this PR, why not record the return value to opensc if a few places later in the user application there is a change in the behavior of the program flow, and here one way or another the error type will show up. But I accept it, in that case I have to modify similarly: |
|
sc_pkcs11_decrypt(): Isn't it a duplicate condition? |
|
I'm afraid that here we would have to push these things through ISO7816-8. I think that from OpenSC's point of view it will be best to prepare (after accepting this PR) patches for each of the supported cards, which will make it possible to turn off hardware depadding on the cards (if possible). MyEID card already has support for turning off depadding in the card, but depadding is turned on by default. This will need to be changed. |
|
Yes, I don't think it needs to be in a single PR. Speaking of wider efforts, I've been talking with Robert Relyea (co-chair of the OASIS PKCS#11 committee) about recommending Marvin workaround/implicit rejection on PKCS#11 level. I'm also working on an update to the PKCS#1 standard (through CFRG) here: https://datatracker.ietf.org/doc/draft-kario-rsa-guidance/ . I don't think I have the necessary contacts, or time, to influence ISO standards directly. |
This should be fixed by now. |
|
The final code is fine, but the readability of this PR (17 commits) is rather unclear. I would like it if the PR could be simplified somehow, after years it will be very difficult to read what was done and why. Don't fix your own code with new commits, use rebase and fix the commit where the error in your PR originated. For example: commit And one whitespace error: coding style: It will be better if we write it down without that space.. Try using clang-format - more in #2017 |
|
Sure, will do, I was just waiting to see if there were any other comments, thanks. |
xhanulik
force-pushed
the
constant-pkcs1
branch
from
d9d3679
to
d0a066b
Compare
xhanulik
force-pushed
the
constant-pkcs1
branch
from
d0a066b
to
7c5dd65
Compare
xhanulik
force-pushed
the
constant-pkcs1
branch
from
7c5dd65
to
a53bfc9
Compare
xhanulik
force-pushed
the
constant-pkcs1
branch
2 times, most recently
from
10f4ddd
to
9f0ade0
Compare
To prevent Marvin attack on RSA PKCS#1 v1.5 padding when logging the return value, signaling the padding error.
In order to not disclose time side-channel when the depadding fails, do the same operations as for case when depadding ends with success.
To prevent Marvin attack on RSA PKCS#1 v1.5 padding when logging the return value, signaling the padding error.
xhanulik
force-pushed
the
constant-pkcs1
branch
from
9f0ade0
to
aa6bf2b
Compare
|
Added minor fixes for includes order. |
|
I have also checked how the returned decrypted buffer is handled in the card drivers (supporting RSA_RAW operation) before the depadding is done. In most drivers, I haven't found any code dependent on the particular bytes in the buffer, so that should be ok. There is only OpenSC/src/libopensc/card-tcos.c Lines 657 to 671 in b21d01e I don't know whether the card checks the padding, and this stripping is done only when the padding is correct, so I'm not sure whether this needs more care or not. |
|
Thank you! Merging to move along with the release. |
This PR implements constant-time PKCS#1 v1.5 depadding and return value handling after RSA decryption. The returned value and depadded message are handled in the same way, both for valid and invalid padding, not to disclose the result of the depadding operation in the time side-channel.
Checklist