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

Do not allow to reuse TOTP passcode #3878

Merged
merged 3 commits into from May 2, 2018

Conversation

@lafriks
Member

lafriks commented May 1, 2018

No description provided.

@lafriks lafriks added this to the 1.5.0 milestone May 1, 2018

@codecov-io

This comment has been minimized.

codecov-io commented May 1, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

@bkcsoft bkcsoft added the lgtm/need 2 label May 1, 2018

@ghost

This comment has been minimized.

ghost commented May 1, 2018

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

@techknowlogick

This comment has been minimized.

Member

techknowlogick commented May 1, 2018

LGTM

edit: noticed a minor typo

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels May 1, 2018

@@ -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),

This comment has been minimized.

@techknowlogick

techknowlogick May 2, 2018

Member

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.

@JonasFranzDEV

LGTM except the typo reported

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels May 2, 2018

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

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

@lafriks

This comment has been minimized.

Member

lafriks commented May 2, 2018

@techknowlogick

This comment has been minimized.

Member

techknowlogick commented May 2, 2018

thanks @lafriks, I've just approved.

lunny and others added some commits May 2, 2018

@daviian

daviian requested changes May 2, 2018 edited

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

This comment has been minimized.

Member

lafriks commented May 2, 2018

@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

daviian approved these changes May 2, 2018

@daviian

This comment has been minimized.

Member

daviian commented May 2, 2018

Don't want to block that PR ;)

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

@techknowlogick

This comment has been minimized.

Member

techknowlogick commented May 2, 2018

@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

This comment has been minimized.

Member

daviian commented May 2, 2018

@techknowlogick @lafriks You are totally right! 👍

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

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

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

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

@lafriks

This comment has been minimized.

Member

lafriks commented May 2, 2018

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

@ghost

This comment has been minimized.

ghost commented May 2, 2018

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

This comment has been minimized.

Member

techknowlogick commented May 2, 2018

@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

This comment has been minimized.

Member

lafriks commented May 3, 2018

@cezar97 finders will get credits in blog post

JonasFranzDEV added a commit to JonasFranzDEV/gitea that referenced this pull request May 6, 2018

@lafriks lafriks added the changelog label Jul 3, 2018

aswild added a commit to aswild/gitea that referenced this pull request Jul 6, 2018

Merge tag 'v1.5.0-rc1' into wild/v1.5
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment