Merge pull request #119 from Cyberwatch/security_fix

Disallow request in the future.
mgomes · Oct 28, 2016 · 912531f · 912531f
2 parents cf35614 + 3a8d792
commit 912531f
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 17 deletions.
diff --git a/lib/api_auth/base.rb b/lib/api_auth/base.rb
@@ -41,7 +41,7 @@ def authentic?(request, secret_key, options = {})
         false
       elsif !signatures_match?(headers, secret_key, options)
         false
-      elsif request_too_old?(headers)
+      elsif !request_within_time_window?(headers)
         false
       else
         true
@@ -71,12 +71,13 @@ def generate_secret_key
 
     AUTH_HEADER_PATTERN = /APIAuth(?:-HMAC-(MD5|SHA(?:1|224|256|384|512)?))? ([^:]+):(.+)$/
 
-    def request_too_old?(headers)
+    def request_within_time_window?(headers)
       # 900 seconds is 15 minutes
 
-      Time.httpdate(headers.timestamp).utc < (Time.now.utc - 900)
+      Time.httpdate(headers.timestamp).utc > (Time.now.utc - 900) &&
+        Time.httpdate(headers.timestamp).utc < (Time.now.utc + 900)
     rescue ArgumentError
-      true
+      false
     end
 
     def signatures_match?(headers, secret_key, options)

diff --git a/spec/api_auth_spec.rb b/spec/api_auth_spec.rb
@@ -75,37 +75,44 @@ def hmac(secret_key, request, canonical_string = nil, digest = 'sha1')
 
   describe '.authentic?' do
     let(:request) do
-      new_request = Net::HTTP::Put.new('/resource.xml?foo=bar&bar=foo',
-                                       'content-type' => 'text/plain',
-                                       'content-md5' => '1B2M2Y8AsgTpgAmY7PhCfg==',
-                                       'date' => Time.now.utc.httpdate)
+      Net::HTTP::Put.new('/resource.xml?foo=bar&bar=foo',
+                         'content-type' => 'text/plain',
+                         'content-md5' => '1B2M2Y8AsgTpgAmY7PhCfg==',
+                         'date' => Time.now.utc.httpdate)
+    end
 
-      signature = hmac('123', new_request)
-      new_request['Authorization'] = "APIAuth 1044:#{signature}"
-      new_request
+    let(:signed_request) do
+      signature = hmac('123', request)
+      request['Authorization'] = "APIAuth 1044:#{signature}"
+      request
     end
 
     it 'validates that the signature in the request header matches the way we sign it' do
-      expect(ApiAuth.authentic?(request, '123')).to eq true
+      expect(ApiAuth.authentic?(signed_request, '123')).to eq true
     end
 
     it 'fails to validate a non matching signature' do
-      expect(ApiAuth.authentic?(request, '456')).to eq false
+      expect(ApiAuth.authentic?(signed_request, '456')).to eq false
     end
 
     it 'fails to validate non matching md5' do
       request['content-md5'] = '12345'
-      expect(ApiAuth.authentic?(request, '123')).to eq false
+      expect(ApiAuth.authentic?(signed_request, '123')).to eq false
     end
 
     it 'fails to validate expired requests' do
       request['date'] = 16.minutes.ago.utc.httpdate
-      expect(ApiAuth.authentic?(request, '123')).to eq false
+      expect(ApiAuth.authentic?(signed_request, '123')).to eq false
+    end
+
+    it 'fails to validate far future requests' do
+      request['date'] = 16.minutes.from_now.utc.httpdate
+      expect(ApiAuth.authentic?(signed_request, '123')).to eq false
     end
 
     it 'fails to validate if the date is invalid' do
       request['date'] = '٢٠١٤-٠٩-٠٨ ١٦:٣١:١٤ +٠٣٠٠'
-      expect(ApiAuth.authentic?(request, '123')).to eq false
+      expect(ApiAuth.authentic?(signed_request, '123')).to eq false
     end
 
     it 'fails to validate if the request method differs' do

diff --git a/spec/railtie_spec.rb b/spec/railtie_spec.rb
@@ -73,7 +73,7 @@ def generated_response(request, action = :index)
       expect(response.code).to eq('200')
     end
 
-    it 'should forbid a request with properly signed headers but timestamp > 15 minutes' do
+    it 'should forbid a request with properly signed headers but timestamp > 15 minutes ago' do
       request = if ActionController::TestRequest.respond_to?(:create)
                   ActionController::TestRequest.create
                 else
@@ -85,6 +85,18 @@ def generated_response(request, action = :index)
       expect(response.code).to eq('401')
     end
 
+    it 'should forbid a request with properly signed headers but timestamp > 15 minutes in the future' do
+      request = if ActionController::TestRequest.respond_to?(:create)
+                  ActionController::TestRequest.create
+                else
+                  ActionController::TestRequest.new
+                end
+      request.env['DATE'] = 'Mon, 23 Jan 2100 03:29:56 GMT'
+      ApiAuth.sign!(request, '1044', API_KEY_STORE['1044'])
+      response = generated_response(request, :index)
+      expect(response.code).to eq('401')
+    end
+
     it "should insert a DATE header in the request when one hasn't been specified" do
       request = if ActionController::TestRequest.respond_to?(:create)
                   ActionController::TestRequest.create