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

totp_verifyi doesn't take digit count into account #9

Closed
feliwir opened this issue Jun 2, 2022 · 2 comments
Closed

totp_verifyi doesn't take digit count into account #9

feliwir opened this issue Jun 2, 2022 · 2 comments

Comments

@feliwir
Copy link
Contributor

feliwir commented Jun 2, 2022

E.g. when having a digitcount of 6 and when the key is "025341", this will be 25341 as an integer. However the conversion from int to string does not care about the digitcount:

char* key_str = calloc(8, sizeof(char));
sprintf(key_str, "%d", key);

This will result in two different strings being compared: "25341" and "025341". I suggest comparing the integer keys instead of the strings. Another issue is that digitcount may never be larger than 7, since key_str is allocated with a fixed size. This should be DIGITCOUNT+1 everywhere. So every calloc(8, sizeof(char)); is potentially wrong within this library

@feliwir feliwir changed the title totpverifyi doesn't take digit count into account totp_verifyi doesn't take digit count into account Jun 2, 2022
@tilkinsc
Copy link
Owner

tilkinsc commented Jun 4, 2022

Yeah this must be where tests are failing. Currently the library was only made to support 6 digit codes. However, your solution to not compare it as a string is wrong. The 0 lacking in the string is a major flaw and is potentially breaking tests, too. 012345 is different than 12345 so I am going to propose I remove the integer checks.

@tilkinsc tilkinsc closed this as completed Jun 9, 2022
@tilkinsc
Copy link
Owner

tilkinsc commented Jun 9, 2022

This issue is explained in #11 and will be referenced from thereof, as this is more or less a duplicate. There is also nothing wrong with calloc(8, ...) as its well defined, as the output is expected and handled. Although, theres issues with the source of it instead.

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

No branches or pull requests

2 participants