Prevent impersonation of public authorities

Quick link to the domain matching regex and some examples:
http://rubular.com/r/UxQwgVxJy6

Thanks to Chris Zetter for the following report:

> I was looking at how Alaveteli/WhatDoTheyKnow worked and noticed a
> potential security issue which I would like to report.
>
> I'm not sure how severe you would consider this particular issue, I
> wanted to share it in private as it could effect existing
> installations of Alaveteli.
>
>
> ## What it allows
>
> The issue allows an attacker to reply to requests as if they belonged
> to the body the request was directed to.
>
> As far as I can see, the attack does not expose any sensitive
> information of registered users.
>
>
> ## Example attack
>
> I've gone through these steps with a local installation of Alaveteli
> to verify. The steps are:
>
> + A body is registered with a FOI email address of 'foi@example.gov.uk'
> + A user submits a request to this body on Alaveteli
> + An attacker registers the domain gsi.uk, and sets up a subdomain of
> 'example.gov.gsi.uk'
> + An attacker creates a user account on Alaveteli with the email
> 'foi@example.gov.gsi.uk'
> + The attacker can then respond to the users request as,
>
>
> ## Why this attack is possible
>
> The `PublicBody#is_foi_officer?` method that checks if a user is
> allowed to respond to requests
> by verifying the email is on the same domain by using the
> `PublicBody.extract_domain_from_email` method.
>
> The `PublicBody.extract_domain_from_email` method replaces certain
> strings ('.gsi.', '.x.' and '.pnn.') with '.'
>
> I'm assuming this code was implemented to allow UK government
> departments to respond to requests
> even if the response from a different email service- for example
> 'example.gsi.gov.uk' and 'example.gov.uk' would
> be considered equivalent.
>
> The problem of this is that it's not always appropriate substitution
> to make, for example, the owner of
> 'example.gov.uk' and 'example.gov.gsi.uk' may be different entities.
>
> I have attached a git patch containing a failing test which demonstrates this.
>
>
> ## Is this attack feasible?
>
> This depends on the domain of the body being targeted and the
> availability of domains.
> I think the attack has become easier in the last few years as more top
> and second level domains (such as '.uk') have become available to
> register subdomains on.
>
>
> ## Prevention
>
> I didn't feel that I could create a patch myself because I don't know
> the original intent of the substitutions and the cases they were meant
> to cover. Removing the substitutions or making them more specific by
> matching on the start or end of the string would prevent the attack
> (such as the regex `/\.gsi\.gov\.uk$/`).
>
>
> I hope that explains the issue, and apologies if I have made any mistakes.
>
> Please contact me if I can be of any further help,
>
> Thanks,
>
> Chris Zetter

    From 32e2e3270e627eb826685c3d858e77bb92a1dd22 Mon Sep 17 00:00:00 2001
    From: Chris Zetter <zetter@users.noreply.github.com>
    Date: Mon, 21 Dec 2015 20:35:32 +0000
    Subject: [PATCH] Failing test that highlights problem with `is_foi_officer?`
     method

    ---
     spec/models/public_body_spec.rb | 19 +++++++++++++++++++
     1 file changed, 19 insertions(+)

    diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb
    index 744598f..8b07c17 100644
    --- a/spec/models/public_body_spec.rb
    +++ b/spec/models/public_body_spec.rb
    @@ -29,6 +29,25 @@
     require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')

     describe PublicBody do
    +  describe '#is_foi_officer?' do
    +    it 'is true for users with emails of the same domain as the public body' do
    +       body = PublicBody.new(request_email: 'david@example.gov.uk')
    +       user = User.new(email: 'george@example.gov.uk')
    +       expect(body.is_foi_officer?(user)).to be(true)
    +    end
    +
    +    it 'is false for users with emails of a different domain to the public body' do
    +       body = PublicBody.new(request_email: 'david@example.gov.uk')
    +       user = User.new(email: 'george@example.co.uk')
    +       expect(body.is_foi_officer?(user)).to be(false)
    +    end
    +
    +    it 'is false when users have registerd domains similar to the public bodies' do
    +       body = PublicBody.new(request_email: 'david@example.gov.uk')
    +       user = User.new(email: 'attacker@example.gov.gsi.uk')
    +       expect(body.is_foi_officer?(user)).to be(false)
    +    end
    +  end

       describe '#translations_attributes=' do

    --
    2.2.1

lib/model_patches.rb

@@ -50,9 +50,11 @@ def self.extract_domain_from_email(email)
         ret = $1.downcase
 
         # remove special email domains for UK Government addresses
-        ret.sub!(".gsi.", ".")
-        ret.sub!(".x.", ".")
-        ret.sub!(".pnn.", ".")
+        %w(gsi x pnn).each do |subdomain|
+          if ret =~ /.*\.*#{ subdomain }\.*.*\.gov\.uk$/
+            ret.sub!(".#{ subdomain }.", '.')
+          end
+        end
 
         return ret
       end

spec/model_patches_spec.rb

@@ -76,6 +76,18 @@
       end
     end
 
+    it 'does not replace addresses similar to uk-specific government domains' do
+      emails = ['attacker@example.gov.gsi.uk',
+                'attacker@example.gov.x.uk',
+                'attacker@example.gov.pnn.uk',
+                'attacker@example.gsi.gov.uk.example.com']
+
+      emails.each do |email|
+        result = PublicBody.extract_domain_from_email(email)
+        expect(result).to_not eq('example.gov.uk')
+      end
+    end
+
   end
 
 end