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

fix totp check by time #28

Closed
taoga opened this issue Dec 6, 2024 · 6 comments
Closed

fix totp check by time #28

taoga opened this issue Dec 6, 2024 · 6 comments

Comments

@taoga
Copy link

taoga commented Dec 6, 2024

Hello!
tdata.verify does not perform totp check by time
To fix this I suggest the following changes. I couldn't upload them to GitHub.

@tilkinsc
Copy link
Owner

I appreciate your concern, but posting zip files like that seems extremely dangerous. Given your account has no history, I have deleted the file. You are free to copy and paste what you meant by tdata.verify does not perform totp check by time here directly. You'll find wrapping the code in backticks useful.

this is an example

```
this is an example
```

@taoga
Copy link
Author

taoga commented Dec 18, 2024

cotp.c

255c255
< 	
---
> 
278c278,279
< 	return otp_generate(data, totp_timecode(data, for_time) + offset, out_str);
---
>     //return otp_generate(data, totp_timecode(data, for_time) + offset, out_str);
>     return otp_generate(data, totp_timecode(data, for_time + offset), out_str);
330c331,332
< 		for (int64_t i=-valid_window; i<valid_window+1; i++)
---
>         //for (int64_t i=-valid_window; i<valid_window+1; i++)
>         for (int64_t i=0; i<valid_window; i++)

diff test/main.cpp ./fix/test/main.cpp
5a6
> #include <thread>
99c100
< 	const int INTERVAL	= 30;
---
>     const int INTERVAL	= 60;
104c105,106
< 	const char BASE32_SECRET[] = "JBSWY3DPEHPK3PXP";
---
> 	//const char BASE32_SECRET[] = "JBSWY3DPEHPK3PXP";
>   const char BASE32_SECRET[] = "SDF32E56MHJKLS3P";
123c125,126
< 		hmac_algo_sha1,
---
>         //hmac_algo_sha1,
>         hmac_algo_sha256,
240a244,248
>     uint64_t cur_time = get_current_time();
>     uint64_t time_shift = cur_time % INTERVAL;
> 
>     cout << "cur_time:" << cur_time << " time_shift:" << time_shift << " whole part:" << cur_time / INTERVAL << endl;
> 
250c258
< 	cout << "totp_now() (padding) pass=1: `" << tcode << "` `" << totp_err_1 << "`" << endl;
---
>     cout << "totp_now() (padding) pass=1: `" << tcode << "` `" << totp_err_1 << "`" << endl;
256c264
< 	int totp_err_2 = tdata.at(0, 0, tcode2);
---
>     int totp_err_2 = tdata.at(0, 0, tcode2);
262c270
< 	cout << "totp_at(0, 0) (padding) pass=1: `" << tcode2 << "` `" << totp_err_2 << "`" << endl;
---
>     cout << "totp_at(0, 0) (padding) pass=1: `" << tcode2 << "` `" << totp_err_2 << "`" << endl;
268,271c276,285
< 	
< 	// Will succeed, timeblock 0 for JBSWY3DPEHPK3PXP == 282760
< 	int tv2 = tdata.verify("282760", 0, 4);
< 	cout << "TOTP Verification 2 (padding) pass=true: `" << (tv2 == 0 ? "false" : "true") << "`" << endl;
---
> 
>     for( int i = 0; i < INTERVAL + 10; i++)
>     {
>         std::this_thread::sleep_for(std::chrono::seconds(1));
>         // Will succeed, timeblock 0 for JBSWY3DPEHPK3PXP == 282760
>         // SDF32E56MHJKLS3P = 897826
>         uint64_t time_slot = (get_current_time() - time_shift) / INTERVAL;
>         int tv2 = tdata.verify(tcode, get_current_time() - time_shift, INTERVAL);
>         cout << "+" << i + 1 << " sec. " << " tslot:" << time_slot << " TOTP Verification 2 (padding) pass=true: `" << (tv2 == 0 ? "false" : "true") << "`" << endl;
>     }
348c362
< 	int hv2 = hdata.compare("996554", 1);
---
>     int hv2 = hdata.compare("629962", 1);

@tilkinsc
Copy link
Owner

You are using the offset parameter incorrectly. The offset is in timeblocks (N / period) rather than seconds, typically 30 second chunks. This offset exists to spread out the verify function to support letting older/future codes to work.

@taoga
Copy link
Author

taoga commented Dec 26, 2024

TOTP should go bad after a time equal to the interval. In your version, TOTP is always valid. With my corrections, it goes bad after a time equal to the interval + 1. Maybe it's worth trying, instead of chatting. :-)

@taoga
Copy link
Author

taoga commented Dec 26, 2024

I don't really care, it's up to you to decide :-)

@tilkinsc
Copy link
Owner

tilkinsc commented Dec 29, 2024

The TOTP is only valid as long as you want it to be valid, given the current time. If a user would like codes to be generated every 30 seconds as is widely common, then they can do so. They just get the added benefit of choosing to accept codes on additional INTERVAL steps, hence passing 1 to the last param of totp_verify will make it so that every code lasts for a full minute. You are passing INTERVAL (90) which makes codes last for 8,100 seconds. This is why I say you are using it incorrectly. For reference, some applications allow codes to exist for up to 10 minutes, which makes sense in terms of generating codes through email where emails can arrive extraordinary late.

// totp_now
char tcode[DIGITS+1];
memset(tcode, 0, DIGITS+1);

int totp_err_1 = totp_now(tdata, tcode);
if(totp_err_1 == OTP_ERROR)
{
	fputs("TOTP Error totp_now", stderr);
	return EXIT_FAILURE;
}
printf("totp_now() pass=1: `%s` `%d`\n", tcode, totp_err_1);

int cnt = 0;
while (cnt < INTERVAL + 10)
{
	uint64_t curTime = get_current_time();
	int tv1 = totp_verify(tdata, tcode, get_current_time(), 0); // 0 means it's only valid for INTERVAL seconds
	printf("TOTP Verification 1_%d_%llu pass=true: `%s`\n", cnt, curTime / INTERVAL, tv1 == 0 ? "false" : "true");
	Sleep(1000);
	cnt++;
}

From this example you can quickly see why it's a problem to use 0 as the offset, as if you generate the code at 28 seconds you only give the user 2 seconds to verify the code, making an interesting situation where you have to wait an extra 2 seconds before typing in the code.

@tilkinsc tilkinsc closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2024
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