Do not allow to reuse TOTP passcode #3878

lafriks · 2018-05-01T18:13:26Z

No description provided.

codecov-io · 2018-05-01T18:21:09Z

Codecov Report

Merging #3878 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3878      +/-   ##
==========================================
- Coverage   20.18%   20.16%   -0.02%     
==========================================
  Files         145      145              
  Lines       29151    29156       +5     
==========================================
- Hits         5883     5880       -3     
- Misses      22374    22381       +7     
- Partials      894      895       +1

Impacted Files	Coverage Δ
models/twofactor.go	`0% <ø> (ø)`	⬆️
routers/user/auth.go	`0% <0%> (ø)`	⬆️
modules/process/manager.go	`69.56% <0%> (-4.35%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c58e1e4...a3bc0c2. Read the comment docs.

ghost · 2018-05-01T20:55:51Z

Thank you for taking care of the security issues reported, even those with lower priority like this one.

techknowlogick · 2018-05-01T20:58:43Z

~~LGTM~~

edit: noticed a minor typo

techknowlogick · 2018-05-02T02:56:48Z

models/migrations/migrations.go

@@ -176,6 +176,8 @@ var migrations = []Migration{
 	NewMigration("add is_fsck_enabled column for repos", addFsckEnabledToRepo),
 	// v61 -> v62
 	NewMigration("add size column for attachments", addSizeToAttachment),
+	// v62 -> v63
+	NewMigration("add last used passcode column for TOPT", addLastUsedPasscodeTOPT),


Incorrect acronym, this should be TOTP (per: https://en.wikipedia.org/wiki/Time-based_One-time_Password_algorithm). There are other places in this PR that have the incorrect acronym.

Sorry for noticing this after my LG-TM, this should be an easy fix though.

jonasfranz

LGTM except the typo reported

lafriks · 2018-05-02T05:46:32Z

@techknowlogick fixed

techknowlogick · 2018-05-02T13:55:54Z

thanks @lafriks, I've just approved.

daviian

Can we just store the hash of it instead of the plain passcode? Seems to be a more trustable approach to solve this.
My gut says this should be handled as sensitive information.

lafriks · 2018-05-02T14:26:25Z

@daviian it is one time usage passcode. Hashing would not give much anyway as it could be easily bruteforced back to normal value (only six digits so that would take few seconds to get value back from hash), I don't think it would change much if anything

daviian · 2018-05-02T14:46:06Z

Don't want to block that PR ;)

But if it's that easy to bruteforce TOTP, why use it in the first place?

techknowlogick · 2018-05-02T14:49:36Z

@daviian I think @lafriks means that it is easy to bruteforce a hash of 6 characters, so if an attacker has access to the DB a hash wouldn't really stop them from knowing what the token was. This is fine because tokens change every 30 seconds, and this is a protection just in case someone MITM a user to intercept the token so it can't be re-used.

daviian · 2018-05-02T14:55:19Z

@techknowlogick @lafriks You are totally right! 👍

lafriks · 2018-05-02T16:37:20Z

As this is more enchantment and it is hard to backport to 1.4 without breaking upgrade process, I will not backport it

ghost · 2018-05-02T22:32:36Z

Personally I don't care too much, but may I ask why finders don't receive any credits? Indeed, it would be really appreciated...

techknowlogick · 2018-05-02T22:47:06Z

@cezar97 I can't speak on behalf of the other maintainers, but I've asked for a thanks to be added to the blog post for the release notes. As only owners see the security reports I wasn't able to add the appropriate thanks to my PR for the release blog post.

lafriks · 2018-05-03T04:03:38Z

@cezar97 finders will get credits in blog post

* SECURITY * Limit uploaded avatar image-size to 4096x3072 by default (go-gitea#4353) * Do not allow to reuse TOTP passcode (go-gitea#3878) * FEATURE * Add cli commands to regen hooks & keys (go-gitea#3979) * Add support for FIDO U2F (go-gitea#3971) * Added user language setting (go-gitea#3875) * LDAP Public SSH Keys synchronization (go-gitea#1844) * Add topic support (go-gitea#3711) * Multiple assignees (go-gitea#3705) * Add protected branch whitelists for merging (go-gitea#3689) * Global code search support (go-gitea#3664) * Add label descriptions (go-gitea#3662) * Add issue search via API (go-gitea#3612) * Add repository setting to enable/disable health checks (go-gitea#3607) * Emoji Autocomplete (go-gitea#3433) * Implements generator cli for secrets (go-gitea#3531) * ENHANCEMENT * Add more webhooks support and refactor webhook templates directory (go-gitea#3929) * Add new option to allow only OAuth2/OpenID user registration (go-gitea#3910) * Add option to use paged LDAP search when synchronizing users (go-gitea#3895) * Symlink icons (go-gitea#1416) * Improve release page UI (go-gitea#3693) * Add admin dashboard option to run health checks (go-gitea#3606) * Add branch link in branch list (go-gitea#3576) * Reduce sql query times in retrieveFeeds (go-gitea#3547) * Option to enable or disable swagger endpoints (go-gitea#3502) * Add missing licenses (go-gitea#3497) * Reduce repo indexer disk usage (go-gitea#3452) * Enable caching on assets and avatars (go-gitea#3376) * Add repository search ordered by stars/forks. Forks column in admin repo list (go-gitea#3969) * Add Environment Variables to Docker template (go-gitea#4012) * LFS: make HTTP auth period configurable (go-gitea#4035) * Add config path as an optionial flag when changing pass via CLI (go-gitea#4184) * Refactor User Settings sections (go-gitea#3900) * Allow square brackets in external issue patterns (go-gitea#3408) * Add Attachment API (go-gitea#3478) * Add EnableTimetracking option to app settings (go-gitea#3719) * Add config option to enable or disable log executed SQL (go-gitea#3726) * Shows total tracked time in issue and milestone list (go-gitea#3341) * TRANSLATION * Improve English grammar and consistency (go-gitea#3614) * DEPLOYMENT * Allow Gitea to run as different USER in Docker (go-gitea#3961) * Provide compressed release binaries (go-gitea#3991) * Sign release binaries (go-gitea#4188)

lafriks added type/enhancement An improvement of existing functionality topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels May 1, 2018

lafriks added this to the 1.5.0 milestone May 1, 2018

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 1, 2018

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 1, 2018

lafriks added the backport/v1.4 label May 1, 2018

techknowlogick requested changes May 2, 2018

View reviewed changes

jonasfranz approved these changes May 2, 2018

View reviewed changes

bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 2, 2018

Do not allow to reuse TOTP passcode

a58db36

lafriks force-pushed the fix/otp_reuse branch from fcfd468 to a58db36 Compare May 2, 2018 05:42

lafriks changed the title ~~Do not allow to reuse TOPT passcode~~ Do not allow to reuse TOTP passcode May 2, 2018

appleboy approved these changes May 2, 2018

View reviewed changes

techknowlogick approved these changes May 2, 2018

View reviewed changes

lunny and others added 2 commits May 2, 2018 21:55

Merge branch 'master' into fix/otp_reuse

0bc5851

Merge branch 'master' into fix/otp_reuse

a3bc0c2

daviian suggested changes May 2, 2018

View reviewed changes

daviian approved these changes May 2, 2018

View reviewed changes

lafriks merged commit 1e1ece8 into go-gitea:master May 2, 2018

lafriks deleted the fix/otp_reuse branch May 2, 2018 15:02

lafriks removed the backport/v1.4 label May 2, 2018

lafriks added the type/changelog Adds the changelog for a new Gitea version label Jul 3, 2018

go-gitea locked and limited conversation to collaborators Nov 24, 2020

delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow to reuse TOTP passcode #3878

Do not allow to reuse TOTP passcode #3878

lafriks commented May 1, 2018

codecov-io commented May 1, 2018 •

edited

Loading

ghost commented May 1, 2018 •

edited by ghost

Loading

techknowlogick commented May 1, 2018 •

edited

Loading

techknowlogick May 2, 2018

jonasfranz left a comment

lafriks commented May 2, 2018

techknowlogick commented May 2, 2018

daviian left a comment •

edited

Loading

lafriks commented May 2, 2018

daviian commented May 2, 2018

techknowlogick commented May 2, 2018

daviian commented May 2, 2018

lafriks commented May 2, 2018

ghost commented May 2, 2018 •

edited by ghost

Loading

techknowlogick commented May 2, 2018

lafriks commented May 3, 2018

Do not allow to reuse TOTP passcode #3878

Do not allow to reuse TOTP passcode #3878

Conversation

lafriks commented May 1, 2018

codecov-io commented May 1, 2018 • edited Loading

Codecov Report

ghost commented May 1, 2018 • edited by ghost Loading

techknowlogick commented May 1, 2018 • edited Loading

techknowlogick May 2, 2018

Choose a reason for hiding this comment

jonasfranz left a comment

Choose a reason for hiding this comment

lafriks commented May 2, 2018

techknowlogick commented May 2, 2018

daviian left a comment • edited Loading

Choose a reason for hiding this comment

lafriks commented May 2, 2018

daviian commented May 2, 2018

techknowlogick commented May 2, 2018

daviian commented May 2, 2018

lafriks commented May 2, 2018

ghost commented May 2, 2018 • edited by ghost Loading

techknowlogick commented May 2, 2018

lafriks commented May 3, 2018

codecov-io commented May 1, 2018 •

edited

Loading

ghost commented May 1, 2018 •

edited by ghost

Loading

techknowlogick commented May 1, 2018 •

edited

Loading

daviian left a comment •

edited

Loading

ghost commented May 2, 2018 •

edited by ghost

Loading