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

Inlined function for constant time buffer comparison #31

Closed
wants to merge 1 commit into from

Conversation

Stienvdh
Copy link

No description provided.

@jovanbulck
Copy link
Member

thanks for the PR! Had a quick look and the approach is indeed constant-time, but I'm not convinced though that the compiler might not mess it up.. I tried:

 29 int SM_FUNC(reader) cmp(char *data1, char* data2, int len)
 30 {
 31     return compare_ct_time(data1, data2, len);
 32 }

produces with clang -Os:

00000058 <cmp>:
  58:	04 12       	push	r4		
  5a:	04 41       	mov	r1,	r4	
  5c:	21 83       	decd	r1		
  5e:	c4 43 ff ff 	mov.b	#0,	-1(r4)	;r3 As==00, 0xffff(r4)
  62:	05 3c       	jmp	$+12     	;abs 0x6e
  64:	7c 4f       	mov.b	@r15+,	r12	
  66:	7c ee       	xor.b	@r14+,	r12	
  68:	c4 dc ff ff 	bis.b	r12,	-1(r4)	;0xffff(r4)
  6c:	3d 53       	add	#-1,	r13	;r3 As==11
  6e:	0d 93       	tst	r13		
  70:	f9 23       	jnz	$-12     	;abs 0x64
  72:	c4 93 ff ff 	tst.b	-1(r4)		;0xffff(r4)
  76:	0f 42       	mov	r2,	r15	
  78:	0f 11       	rra	r15		
  7a:	1f f3       	and	#1,	r15	;r3 As==01
  7c:	21 53       	incd	r1		
  7e:	34 41       	pop	r4		
  80:	30 41       	ret	

that looks okay afaik, but I didn't try for other optimization levels etc. I see you annotated everything as volatile. Is this to try and prevent the compiler to optimize things out? If so, better to add documentation for that so it doesn't get removed in future! Also, you might want to add explicit optimize compiler attributes? I'm most worried about the final return (result == 0x0); which might get compiled to an if branch I think

@jovanbulck
Copy link
Member

looked a bit further and this implementation from NaCL looks better:

https://github.com/jedisct1/libsodium/blob/451bafc0d3d95d18f916dd7051687d343597228c/src/libsodium/crypto_verify/sodium/verify.c#L65

As per the ISC license, you can take over this code and rename it for our needs, but then maybe include it in a separate header file with the necessarily licenses info in comment.

@jovanbulck
Copy link
Member

closing now, this has been implemented and merged as per #37

@jovanbulck jovanbulck closed this Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants