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

ROTP::Base32 doesn't properly handle padding #29

Closed
pstengel opened this issue May 27, 2014 · 4 comments
Closed

ROTP::Base32 doesn't properly handle padding #29

pstengel opened this issue May 27, 2014 · 4 comments

Comments

@pstengel
Copy link

Padding using = is supported by RFC3548.

Example:

irb(main):003:0> require "rotp"
=> true
irb(main):004:0> require "base32"
=> true
irb(main):005:0> Base32.encode("thisstringresultsinpadding")
=> "ORUGS43TORZGS3THOJSXG5LMORZWS3TQMFSGI2LOM4======"
irb(main):006:0> Base32.decode("ORUGS43TORZGS3THOJSXG5LMORZWS3TQMFSGI2LOM4======")
=> "thisstringresultsinpadding"
irb(main):007:0> ROTP::Base32.decode("ORUGS43TORZGS3THOJSXG5LMORZWS3TQMFSGI2LOM4======")
ROTP::Base32::Base32Error: Invalid Base32 Character - '='
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:46:in `decode_quint'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:28:in `block in decode_block'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:28:in `each_char'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:28:in `each'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:28:in `map'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:28:in `decode_block'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:10:in `block in decode'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:9:in `each'
    from /Users/pstengel/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rotp-1.6.1/lib/rotp/base32.rb:9:in `decode'
    from (irb):7
    from /Users/pstengel/.rbenv/versions/2.1.2/bin/irb:11:in `<main>'
irb(main):008:0> ROTP::Base32.decode("ORUGS43TORZGS3THOJSXG5LMORZWS3TQMFSGI2LOM4")
=> "thisstringresultsinpadding"
@mdp
Copy link
Owner

mdp commented May 27, 2014

Actually we use a custom version of Base32 on purpose. The Base32 decoder on the most popular 2FA application (Google Authenticator) is a bit wonky. For example, it silently discards extra bits on a Base32 string thats padded - https://code.google.com/p/google-authenticator/source/browse/src/com/google/android/apps/authenticator/Base32String.java?repo=android#101

There's no easy way to go back and fix this.

@mdp mdp closed this as completed May 27, 2014
@pstengel
Copy link
Author

Odd. What's your suggested workaround then for non-random base32 secrets that result in padding? Just drop the = from the generated base32 before passing to ROTP?

@mdp
Copy link
Owner

mdp commented May 27, 2014

So because it's non-standard, I'd actually stick to secret lengths that are multiples of 8. For example, Google uses 16 characters, which works regardless of Base32 behavior. So if another popular 2FA application comes up and handles it differently, they'll both work. Dropbox however uses 20 characters without padding that results in bits being dropped on Google Authenticator.

For perfect future proofing, just use 16 or 24 characters and you should be fine.

@mdp
Copy link
Owner

mdp commented May 27, 2014

Sorry, just realized you said non-random. I'd find a way to make sure it's always 16 or 24 characters long, deterministically.

Off the top of my head, I'd generate a SHA-256. Take the first 24 bytes, then generate a character from Base32 for each one. Something like Base32Chars[byte % 32]. Since 256 is evenly divisible by 32, you don't need to worry about a bias.

Hope that helps

@mdp mdp mentioned this issue Jan 29, 2015
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

Successfully merging a pull request may close this issue.

2 participants