From d16350bc73cd891f9d3bdfd733bb14c9fd23ed53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sta=C5=9Bczak?= Date: Thu, 28 Dec 2023 13:39:18 +0100 Subject: [PATCH] Fix `secret` not being case-insensitive Fixes #80. * * * Example secret provided by Google when trying to enable MFA for Google account (with the note that spaces can be ignored): ``` qea4 ptmn kvd5 ampv wm6d rc6m bbh4 urbo ``` Example respective URI: ``` otpauth://totp/Google%3Adaniel%40buttercms.com?secret=qea4ptmnkvd5ampvwm6drc6mbbh4urbo&issuer=Google ``` Result: ``` $ otp add uri google Enter URI: Repeat for confirmation: Usage: otp add uri [OPTIONS] ALIAS Try 'otp add uri -h' for help. Error: invalid Base32 value; Non-base32 digit found ``` This _is_ Base32-encoded, **however**, Google ignores the [official spec](https://datatracker.ietf.org/doc/html/rfc3548#section-5) that instructs to use **uppercase** letters for Base32-encoded value. On the other hand, the spec also mentions case insensitivity of the value: > The Base 32 encoding is designed to represent arbitrary sequences of > octets in a form that needs to be case insensitive So maybe the issue comes from the spec being ambiguous. We should make the `secret` parameter case-insensitive. --- onetimepass/base32.py | 24 ++++++++++++++++++++++++ onetimepass/db/models.py | 4 ++-- onetimepass/otp.py | 8 ++++---- 3 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 onetimepass/base32.py diff --git a/onetimepass/base32.py b/onetimepass/base32.py new file mode 100644 index 0000000..8677ce8 --- /dev/null +++ b/onetimepass/base32.py @@ -0,0 +1,24 @@ +import base64 +import functools + +# > Optional casefold is a flag specifying whether a lowercase alphabet is +# > acceptable as input. For security purposes, the default is False. +# > —https://docs.python.org/3/library/base64.html#base64.b32decode +# +# We prefer to avoid using something that is disabled by default for security +# purposes, but some services, unfortunately, provide the secret in lowercase +# (e.g. Google!), so not doing that would make the application not working for +# those. +# +# The reason that they provide the secret in lowercase may come from the +# official spec being ambiguous: +# +# > The Base 32 encoding is designed to represent arbitrary sequences of octets +# > in a form that needs to be **case insensitive** +# > […] +# > These characters […] are selected from US-ASCII digits and **uppercase letters**. +# > —https://datatracker.ietf.org/doc/html/rfc3548#section-5 +# +# It mentions using uppercase letters, but also the value being case-insensitive. +# The reason is just a guess, though +decode = functools.partial(base64.b32decode, casefold=True) diff --git a/onetimepass/db/models.py b/onetimepass/db/models.py index 3ec76ce..c293a11 100644 --- a/onetimepass/db/models.py +++ b/onetimepass/db/models.py @@ -1,12 +1,12 @@ from __future__ import annotations -import base64 import binascii import datetime import typing from pydantic import validator +from onetimepass import base32 from onetimepass import settings from onetimepass.base_model import BaseModel from onetimepass.db import exceptions @@ -81,7 +81,7 @@ class AliasSchema(BaseModel): @validator("secret") def valid_base32_secret(cls, v): try: - base64.b32decode(v) + base32.decode(v) except binascii.Error as e: raise ValueError(f"invalid Base32 value; {e}") return v diff --git a/onetimepass/otp.py b/onetimepass/otp.py index fb791fb..f8d460b 100644 --- a/onetimepass/otp.py +++ b/onetimepass/otp.py @@ -1,4 +1,3 @@ -import base64 import binascii import datetime import functools @@ -12,6 +11,7 @@ import pydantic from rich.console import Console +from onetimepass import base32 from onetimepass import master_key from onetimepass import otp_algorithm from onetimepass import settings @@ -158,7 +158,7 @@ def show(ctx: click.Context, alias: str, wait: int | None, minimum_verbose: bool if wait is not None: remaining_seconds = otp_algorithm.get_seconds_remaining( otp_algorithm.TOTPParameters( - secret=base64.b32decode(alias_data.secret), + secret=base32.decode(alias_data.secret), digits_count=alias_data.digits_count, hash_algorithm=alias_data.hash_algorithm, time_step_seconds=alias_data.params.time_step_seconds, @@ -169,7 +169,7 @@ def show(ctx: click.Context, alias: str, wait: int | None, minimum_verbose: bool time.sleep(remaining_seconds) # Reinitialize parameters to get valid result params = otp_algorithm.TOTPParameters( - secret=base64.b32decode(alias_data.secret), + secret=base32.decode(alias_data.secret), digits_count=alias_data.digits_count, hash_algorithm=alias_data.hash_algorithm, time_step_seconds=alias_data.params.time_step_seconds, @@ -187,7 +187,7 @@ def show(ctx: click.Context, alias: str, wait: int | None, minimum_verbose: bool elif alias_data.otp_type == OTPType.HOTP: alias_data.params.counter += 1 params = otp_algorithm.HOTPParameters( - secret=base64.b32decode(alias_data.secret), + secret=base32.decode(alias_data.secret), digits_count=alias_data.digits_count, hash_algorithm=alias_data.hash_algorithm, counter=alias_data.params.counter,