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

MIPS RSA pk encryption produces invalid result #1722

Closed
jmartin-tech opened this issue Jun 12, 2018 · 6 comments · Fixed by #1949
Closed

MIPS RSA pk encryption produces invalid result #1722

jmartin-tech opened this issue Jun 12, 2018 · 6 comments · Fixed by #1949
Labels
bug component-crypto Crypto primitives and low-level interfaces

Comments

@jmartin-tech
Copy link
Contributor

May be related to #1093 and seen impacting big and little endian MIPS

Description


Bug

OS
Mbed OS linux MIPS and MIPSEL

mbed TLS build:
Version: 2.6.0 and still present in 2.9.0
OS version: reproduced in qemu-mips and Openwrt 15.05 (Broadcom BCM5300)
Configuration: attached config.h
config.h.txt

Compiler and options (if you used a pre-built binary, please indicate how you obtained it):
https://bitbucket.org/GregorR/musl-cross

Peer device TLS stack and version
N/A failure can be reproduced in sample code, although this was identified by encrypting data with an RSA public key on MIPS and attempting to decrypt in Ruby 2.5.1 on x86_64.
RSA key was generated with OpenSSL 1.0.2j

openssl req -new -newkey rsa:2048 -nodes -x509 -subj '/CN=localhost' -days 365 -keyout server.key -out server.crt && cat server.key server.crt > server.pem
openssl rsa -in server.pem -pubout > server.pub

Expected behavior
Sample code should report success on decrypt of public key encrypted data.

qemu-mips ./test
loaded public key
ALL IS WELL IN LOCAL DECRYPT

Actual behavior
Decryption fails with error code

qemu-mips ./test
loaded public key
FAILED TO DECRYPT mbedtls_pk_decrypt RETURNED -16640

Steps to reproduce
Sample failing code on MIPS, this code reports success on ARM & x86_64

#include <stdlib.h>
#include <mbedtls/aes.h>
#include <mbedtls/entropy.h>
#include <mbedtls/ctr_drbg.h>
#include <mbedtls/pk.h>

int main (argv)
{
	size_t olen = 0;
	size_t data_len = 16;
	unsigned char data[16] = "ABCDEFGHIJKLMNO";
	mbedtls_pk_context pk;
	mbedtls_ctr_drbg_context ctr_drbg;
	mbedtls_entropy_context entropy;

	mbedtls_entropy_init(&entropy);
	mbedtls_ctr_drbg_init(&ctr_drbg);

	mbedtls_pk_init(&pk);
	if (!(mbedtls_pk_parse_public_keyfile( &pk, "server.pub" ))) {
		printf("loaded public key\n");
		if (!(mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, &entropy, NULL, 0))) {
			mbedtls_pk_context priv_k;
			mbedtls_pk_init(&priv_k);
			unsigned char buf[MBEDTLS_MPI_MAX_SIZE] = {'\0'};
			if (!(mbedtls_pk_encrypt(&pk, data, data_len, buf, &olen, sizeof(buf), mbedtls_ctr_drbg_random, &ctr_drbg))) {
				// now try to decrypt
				if (!(mbedtls_pk_parse_keyfile( &priv_k, "server.key", "" ))) {
					size_t dec_len = 0;
					unsigned char result[MBEDTLS_MPI_MAX_SIZE] = {'\0'};
					int ret = 0;
					if(!(ret = mbedtls_pk_decrypt( &priv_k, buf, olen, result, &dec_len, sizeof(result),
										mbedtls_ctr_drbg_random, &ctr_drbg)))
					{
						printf("ALL IS WELL IN LOCAL DECRYPT\n");
					}
					else
					{
						printf("FAILED TO DECRYPT mbedtls_pk_decrypt RETURNED %d\n", ret);
					}
				}
				else
				{
					printf("FAILED TO PARSE THE PRIVATE KEY\n");
				}
 			} else {
				printf("FAILED TO ENCRYPT DATA\n");
			}
		}

	} else
	{
		printf("FAILED TO PARSE THE PUBLIC KEY\n");
	}
	mbedtls_ctr_drbg_free(&ctr_drbg);
	mbedtls_entropy_free(&entropy);
        return 0;
}

compiled with sample mips-linux-gnu-gcc -static main.c -o test -I"../mbedtls/include/" -L"../lib" -lmbedtls -lmbedcrypto using https://hub.docker.com/r/asmimproved/qemu-mips/

@simonbutcher simonbutcher added bug tracking component-crypto Crypto primitives and low-level interfaces labels Jun 13, 2018
@simonbutcher
Copy link
Contributor

Thanks for the feedback! MIPS isn't a platform we have in our CI nor one we have tools or boards for, so we'd very much welcome a fix for this one.

@simonbutcher simonbutcher added the help-wanted This issue is not being actively worked on, but PRs welcome. label Jun 13, 2018
@ciarmcom
Copy link

ARM Internal Ref: IOTSSL-2364

@aurel32
Copy link
Contributor

aurel32 commented Jun 13, 2018

@jmartin-r7, if the issue is in the bn_mul.h code as suggested by the fact that the disabling the corresponding assembly code workarounds the issue, you might want to try the following patch:

diff --git a/include/mbedtls/bn_mul.h b/include/mbedtls/bn_mul.h
index f4b2b56..5ed46d9 100644
--- a/include/mbedtls/bn_mul.h
+++ b/include/mbedtls/bn_mul.h
@@ -727,7 +727,7 @@
         "sw     $10, %2         \n\t"   \
         : "=m" (c), "=m" (d), "=m" (s)                      \
         : "m" (s), "m" (d), "m" (c), "m" (b)                \
-        : "$9", "$10", "$11", "$12", "$13", "$14", "$15"    \
+        : "$9", "$10", "$11", "$12", "$13", "$14", "$15", "$lo", "$hi" \
     );
 
 #endif /* MIPS */

@jmartin-tech
Copy link
Contributor Author

@aurel32 that does look to address the specific issue in my test, I will do more testing and see about opening a PR. I need to understand the possible impacts before accepting this as a long term solution.

@hanno-becker
Copy link

Hi @jmartin-r7,

I got aware of this issue and the subsequent PR only now: Many thanks for reporting this and for providing a thorough description of how to reproduce the issue (for the record: I had to set CFLAGS="-O3 -march=mips32" when compiling to be able to observe the issue)! As you have seen, I looked into this issue some time ago and failed to reproduce it, so your input is very valuable.

Regards,
Hanno

@hanno-becker hanno-becker added fix available and removed help-wanted This issue is not being actively worked on, but PRs welcome. labels Jan 2, 2019
@hanno-becker
Copy link

Hi @aurel32,

thank you very much for finding and reporting the cause for the issue. I confirm that the clobber list is broken, and that changing it per your suggestion at least fixes the issue at hand in the setup @jmartin-r7 describes. Further review and discussion will be done in PR #1949.

Regards,
Hanno

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants