CRC check broken, silently appears to work

[GoogleCodeExporter]

What steps will reproduce the problem?
- Just an observation from looking at yubiserve.py

OTPValidation::CRC is missing a line when compared to yubico-c's yubikey_crc16 
function.  That C function does this before looping through each bit:
m_crc ^= (uint8_t) * buf++ & 0xFF;

Notice that OTPValidation::CRC does not consider the value of the variable "b" 
after assigning it.  My suggested change to the function is this line: crc = 
crc ^ (b & 0xff), immediately after assigning b.

A second bug obscures this, in OTPValidation::validateOTP:

if not (self.CRC() or self.isCRCValid()):

That line is going to short-circuit evaluate, because self.CRC() will always 
return a non-false value, so self.isCRCValid() will never be evaluated.

Original issue reported on code.google.com by gjo...@mieweb.com on 26 Oct 2012 at 6:06

Attachments:

• yubiserve.patch

[GoogleCodeExporter]

Hi,

Do you have an example test that causes this problem to occur?

Thanks,
Glen

Original comment by glen.ogilvie@gmail.com on 20 Dec 2012 at 2:08

• Changed state: Accepted

[GoogleCodeExporter]

I tried applying your patch, and running the test script, which is located in 
the testing directory, in SVN.  The test script performs a number of actions.

I found that when I update the statement to:
if not self.CRC() or not self.isCRCValid():

The test fails. oddly with no output from the curl command, rather than the 
expected output or an error.  So, changing this if statement, might be exposing 
a bug somewhere else.

Not sure why yet.

Original comment by glen.ogilvie@gmail.com on 2 Jan 2013 at 11:01

[GoogleCodeExporter]

I've got the test running in my environment now.  I believe the crash you are 
seeing is because OTPValidation::isCRCValid is looking at self.crc, but should 
be looking at self.OTPcrc instead, since OTPValidation::CRC sets self.OTPcrc, 
not self.crc (maybe to avoid confusion between self.crc as a property and 
self.CRC as a method).

Even with that and the other changes I suggested, I get BAD_OTP, so I'm going 
to run through some of the CRC calculations by hand to see what's going on.

Original comment by gjo...@mieweb.com on 2 Jan 2013 at 8:02

[GoogleCodeExporter]

Ok, I've got a patch file here that makes the test pass.  My previous test 
accidentally omitted the crc = crc ^ (b & 0xff) change.

Original comment by gjo...@mieweb.com on 2 Jan 2013 at 8:08

Attachments:

• yubi.patch

[GoogleCodeExporter]

Here is a patch for basic_test.sh that includes a new test that should come up 
with BAD_OTP due to an incorrect CRC.  Using the old code, it comes up with 
REPLAYED_OTP.

Original comment by gjo...@mieweb.com on 2 Jan 2013 at 9:25

Attachments:

• test.patch

[GoogleCodeExporter]

Good work.  Both patches applied to SVN, revision 55.   Thank you.

Original comment by glen.ogilvie@gmail.com on 7 Jan 2013 at 3:20

• Changed state: Fixed