Skip to content

Commit

Permalink
Allow for multiple signing errors, and propagate onto login_event.
Browse files Browse the repository at this point in the history
Rather than having the validator issue the 403 on improper signing, this moves the handling into the controller and therefore allows signing errors to be logged as part of a failed login_event.

Additionally, some persistent time-related issues with android's v1 implementation needs debugging.  This disables the epoch check but will still persist the error and timing info on the login_event.
KeyserSosa committed Jul 6, 2016
1 parent 47c7922 commit 36990c4
Showing 6 changed files with 200 additions and 40 deletions.
8 changes: 8 additions & 0 deletions r2/r2/controllers/login.py
Original file line number Diff line number Diff line change
@@ -47,6 +47,10 @@ def _event(error):
request=request,
context=c)

if signature and not signature.is_valid():
_event(error="SIGNATURE")
abort(403)

hook_error = hooks.get_hook("account.login").call_until_return(
responder=responder,
request=request,
@@ -98,6 +102,10 @@ def _event(error):
request=request,
context=c)

if signature and not signature.is_valid():
_event(error="SIGNATURE")
abort(403)

if responder.has_errors('user', errors.USERNAME_TOO_SHORT):
_event(error='USERNAME_TOO_SHORT')

9 changes: 9 additions & 0 deletions r2/r2/lib/eventcollector.py
Original file line number Diff line number Diff line change
@@ -32,6 +32,8 @@
import time

import httpagentparser
import time

from pylons import app_globals as g
from uuid import uuid4
from wsgiref.handlers import format_date_time
@@ -728,6 +730,13 @@ def login_event(self, action_name, error_msg,
event.add("signed", True)
event.add("signature_platform", signature.platform)
event.add("signature_version", signature.version)
event.add("signature_valid", signature.is_valid())
sigerror = ", ".join(
"%s_%s" % (field, code) for code, field in signature.errors
)
event.add("signature_errors", sigerror)
if signature.epoch:
event.add("signature_age", int(time.time()) - signature.epoch)

self.save_event(event)

179 changes: 150 additions & 29 deletions r2/r2/lib/signing.py
Original file line number Diff line number Diff line change
@@ -19,7 +19,7 @@
# All portions of the code written by reddit are Copyright (c) 2006-2015 reddit
# Inc. All Rights Reserved.
###############################################################################
"""Module for request (and eventually cookie) signing.
"""Module for request signing.
"""
import hmac
@@ -31,7 +31,7 @@
from collections import namedtuple
from pylons import app_globals as g

from r2.lib.utils import Storage, epoch_timestamp, constant_time_compare
from r2.lib.utils import Storage, epoch_timestamp, constant_time_compare, tup

GLOBAL_TOKEN_VERSION = 1
SIGNATURE_UA_HEADER = "X-hmac-signed-result"
@@ -42,6 +42,7 @@
r"^(?P<platform>.+?):(?P<version>\d+?):(?P<epoch>\d+?):(?P<mac>.*)$"
)


ERRORS = Storage()
SignatureError = namedtuple("SignatureError", "code msg")
for code, msg in (
@@ -52,11 +53,118 @@
("INVALIDATED_TOKEN", "platform/version combination is invalid."),
("EXPIRED_TOKEN", "epoch provided is too old."),
("SIGNATURE_MISMATCH", "the payload's signature doesn't match the header"),
("MULTISIG_MISMATCH", "more than one version on multiple signatures!")
):
code = code.upper()
ERRORS[code] = SignatureError(code, msg)


class SigningResult(object):
"""
"""
__slots__ = ["global_version", "platform", "version",
"mac", "valid_hmac", "epoch", "ignored_errors", "errors"]

def __init__(
self,
global_version=-1,
platform=None,
version=-1,
mac=None,
valid_hmac=False,
epoch=None,
):
self.global_version = global_version
self.platform = platform
self.version = version
self.mac = mac
self.valid_hmac = valid_hmac
self.epoch = epoch
self.ignored_errors = []
self.errors = {}

def __repr__(self):
return "<%s (%s)>" % (
self.__class__.__name__,
", ".join("%s=%r" % (k, getattr(self, k)) for k in self.__slots__)
)

def add_error(self, error, field=None, details=None):
"""Add an error.
Duplicate errors (those with the same code and field) will have the
last `details` stored.
:param error: The error to be set.
:param field: where the error came from (generally "body" or "ua")
:param details: additional error info (for the event)
:type error: :py:class:`SignatureError`
:type field: str or None
:type details: object
"""
self.errors[(error.code, field)] = details

def add_ignore(self, ignored_error):
"""Add error to list of ignored errors.
:param ignored_error: error to be ignored.
:type ignored_error: :py:class:`SignatureError`
"""
self.ignored_errors.append(ignored_error)

def has_errors(self):
"""Determines if the signature has any errors.
:returns: whether or not there are non-ignored errors
:rtype: bool
"""
if self.ignored_errors:
igcodes = {err.code for err in tup(self.ignored_errors)}
error_codes = {code for code, _ in self.errors}
return not error_codes.issubset(igcodes)
else:
return bool(self.errors)

def is_valid(self):
"""Returns if the hmac is valid and the signature has no errors.
:returns: whether or not this is valid
:rtype: bool
"""
return self.valid_hmac and not self.has_errors()

def update(self, other):
"""Destructively merge this result with another.
the signatures are combined as needed to generate a final signature
that is generally the combination of the two as follows:
- `errors` are combined
- `global_version`, `platform`, and `version` are compared. In the
case of a mismatch, "MULTISIG_MISMATCH" error is set.
- signature validity is independently checked with :py:meth:`is_valid`
:param other: other result to be merged from
:type other: :py:class:`SigningResult`
"""
assert isinstance(other, SigningResult)

# copy errors onto self
self.errors.update(other.errors)

# verify both signatures share versioning info (if they don't,
# something is _very weird_)
for attr in ("global_version", "platform", "version"):
if getattr(self, attr) != getattr(other, attr):
self.add_error(ERRORS.MULTISIG_MISMATCH, details=attr)
break

# also the signature is valid only if both hmacs are valid
self.valid_hmac = self.valid_hmac and other.valid_hmac


def current_epoch():
return int(epoch_timestamp(datetime.now(pytz.UTC)))

@@ -116,7 +224,8 @@ def valid_post_signature(request, signature_header=SIGNATURE_BODY_HEADER):
"Validate that the request has a properly signed body."
return valid_signature(
"Body:{}".format(request.body),
request.headers.get(signature_header)
request.headers.get(signature_header),
field="body",
)


@@ -130,10 +239,14 @@ def valid_ua_signature(
"{}:{}".format(h, request.headers.get(h) or "")
for h in signed_headers
)
return valid_signature(payload, request.headers.get(signature_header))
return valid_signature(
payload,
request.headers.get(signature_header),
field="ua",
)


def valid_signature(payload, signature):
def valid_signature(payload, signature, field=None):
"""Checks if `signature` matches `payload`.
`Signature` (at least as of version 1) be of the form:
@@ -150,77 +263,85 @@ def valid_signature(payload, signature):
per app build as needs be.
* signature is the hmac of the request's POST body with the token derived
from the above three parameters via `get_secret_token`
:param str payload: the signed data
:param str signature: the signature of the payload
:param str field: error field to set (one of "ua", "body")
:returns: object with signature validity and any errors
:rtype: :py:class:`SigningResult`
"""
result = Storage(
global_version=-1,
platform=None,
version=-1,
mac=None,
valid=False,
epoch=None,
error=ERRORS.UNKNOWN,
)
result = SigningResult()

# if the signature is unparseable, there's not much to do
sig_match = SIG_HEADER_RE.match(signature or "")
if not sig_match:
result.error = ERRORS.INVALID_FORMAT
result.add_error(ERRORS.INVALID_FORMAT, field=field)
return result

sig_header_dict = sig_match.groupdict()
# we're matching \d so this shouldn't throw a TypeError
result.global_version = int(sig_header_dict['global_version'])

# incrementing this value is drastic. We can't validate a token protocol
# we don't understand.
if result.global_version > GLOBAL_TOKEN_VERSION:
result.error = ERRORS.UNKOWN_GLOBAL_VERSION
result.add_error(ERRORS.UNKOWN_GLOBAL_VERSION, field=field)
return result

# currently there's only one version, but here's where we'll eventually
# patch in more.
sig_match = SIG_CONTENT_V1_RE.match(sig_header_dict['payload'])
if not sig_match:
result.error = ERRORS.UNPARSEABLE
result.add_error(ERRORS.UNPARSEABLE, field=field)
return result

result.update(sig_match.groupdict())
result.version = int(result.version)
result.epoch = int(result.epoch)
# slop the matched data over to the SigningResult
sig_match_dict = sig_match.groupdict()
result.platform = sig_match_dict['platform']
result.version = int(sig_match_dict['version'])
result.epoch = int(sig_match_dict['epoch'])
result.mac = sig_match_dict['mac']

# verify that the token provided hasn't been invalidated
if is_invalid_token(result.platform, result.version):
result.error = ERRORS.INVALIDATED_TOKEN
result.add_error(ERRORS.INVALIDATED_TOKEN, field=field)
return result

# check the epoch validity, but don't fail -- leave that up to the
# validator!
if not valid_epoch(result.platform, result.epoch):
result.error = ERRORS.EXPIRED_TOKEN
return result
result.add_error(
ERRORS.EXPIRED_TOKEN,
field=field,
details=result.epoch,
)

# get the expected secret used to verify this request.
secret_token = get_secret_token(
result.platform,
result.version,
global_version=result.global_version,
)
result.valid = constant_time_compare(

result.valid_hmac = constant_time_compare(
result.mac,
versioned_hmac(
secret_token,
epoch_wrap(result.epoch, payload),
result.global_version
),
)
if result.valid:
result.error = None
else:
result.error = ERRORS.SIGNATURE_MISMATCH

if not result.valid_hmac:
result.add_error(ERRORS.SIGNATURE_MISMATCH, field=field)

return result


def sign_v1_message(body, platform, version, epoch=None):
"""Reference implementation of the v1 mobile body signing."""
token = get_secret_token(platform, version, global_version=1)
epoch = epoch or current_epoch()
epoch = int(epoch or current_epoch())
payload = epoch_wrap(epoch, body)
signature = versioned_hmac(token, payload, global_version=1)
return "{global_version}:{platform}:{version}:{epoch}:{signature}".format(
35 changes: 26 additions & 9 deletions r2/r2/lib/validator/validator.py
Original file line number Diff line number Diff line change
@@ -3253,18 +3253,35 @@ def param_docs(self):


class VSigned(Validator):
"""Validate if the request is properly signed.
Checks the headers (mostly the User-Agent) are signed with
:py:function:`~r2.lib.signing.valid_ua_signature` and in the case
of POST and PUT ensure that any request.body included is also signed
via :py:function:`~r2.lib.signing.valid_body_signature`.
In :py:method:`run`, the signatures are combined as needed to generate a
final signature that is generally the combination of the two.
"""

def run(self):
ua_signature = signing.valid_ua_signature(request)
if not ua_signature.valid:
g.stats.simple_event(
"signing.ua.invalid.%s" % ua_signature.error.code.lower())
abort(403, 'forbidden')
signature = signing.valid_ua_signature(request)

# only check the request body when there should be one
if request.method.upper() in ("POST", "PUT"):
signature.update(signing.valid_post_signature(request))

signature = signing.valid_post_signature(request)
if not signature.valid:
# add a simple event for each error as it appears (independent of
# whether we're going to ignore them).
for code, field in signature.errors:
g.stats.simple_event(
"signing.body.invalid.%s" % signature.error.code.lower())
abort(403, 'forbidden')
"signing.%s.invalid.%s" % (field, code.lower())
)

# persistent skew problems on android suggest something deeper is
# wrong in v1. Disable the expiration check for now!
if signature.platform == "android" and signature.version == 1:
signature.add_ignore(signing.ERRORS.EXPIRED_TOKEN)

return signature

2 changes: 2 additions & 0 deletions r2/r2/tests/functional/controller/login/post_tests.py
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@
from r2.tests import RedditControllerTestCase
from r2.lib.errors import error_list
from r2.lib.unicode import _force_unicode
from r2.models import Subreddit
from common import LoginRegBase


@@ -33,6 +34,7 @@ class PostLoginRegTests(LoginRegBase, RedditControllerTestCase):

def setUp(self):
super(PostLoginRegTests, self).setUp()
self.autopatch(Subreddit, "_byID", return_value=[])
self.dest = "/foo"

def assert_success(self, res):
7 changes: 5 additions & 2 deletions r2/r2/tests/unit/lib/signing_tests.py
Original file line number Diff line number Diff line change
@@ -44,8 +44,11 @@ def _assert_validity(self, body, header, success, error, **expected):
if header:
request.headers[signing.SIGNATURE_BODY_HEADER] = header
signature = signing.valid_post_signature(request)
self.assertEqual(bool(signature.valid), bool(success))
self.assertEqual(signature.error, error)
self.assertEqual(signature.is_valid(), bool(success))
if error:
self.assertIn(error.code, [code for code, _ in signature.errors])
else:
self.assertEqual(len(signature.errors), 0)
has_mac = expected.pop("has_mac", False)
for k, v in expected.iteritems():
got = getattr(signature, k)

0 comments on commit 36990c4

Please sign in to comment.