From 3c566399cf8dab3aff8c54381e7b83b0e6ef3995 Mon Sep 17 00:00:00 2001 From: David Broder-Rodgers Date: Fri, 23 Dec 2016 11:29:31 +0000 Subject: [PATCH 1/4] Added check for insecure mirror URLs --- Library/Homebrew/dev-cmd/audit.rb | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index e83fb2bd07..fffe14b47a 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -623,11 +623,11 @@ class FormulaAuditor %w[Stable Devel HEAD].each do |name| next unless spec = formula.send(name.downcase) - ra = ResourceAuditor.new(spec).audit + ra = ResourceAuditor.new(spec, online: @online).audit problems.concat ra.problems.map { |problem| "#{name}: #{problem}" } spec.resources.each_value do |resource| - ra = ResourceAuditor.new(resource).audit + ra = ResourceAuditor.new(resource, online: @online).audit problems.concat ra.problems.map { |problem| "#{name} resource #{resource.name.inspect}: #{problem}" } @@ -1127,7 +1127,7 @@ class ResourceAuditor attr_reader :problems attr_reader :version, :checksum, :using, :specs, :url, :mirrors, :name - def initialize(resource) + def initialize(resource, options = {}) @name = resource.name @version = resource.version @checksum = resource.checksum @@ -1135,6 +1135,7 @@ class ResourceAuditor @mirrors = resource.mirrors @using = resource.using @specs = resource.specs + @online = options[:online] @problems = [] end @@ -1390,6 +1391,20 @@ class ResourceAuditor next unless u =~ %r{https?://(?:central|repo\d+)\.maven\.org/maven2/(.+)$} problem "#{u} should be `https://search.maven.org/remotecontent?filepath=#{$1}`" end + + return unless @online + urls.each do |url| + next unless url.start_with? "http:" + # Check for insecure mirrors + status_code, = curl_output "--connect-timeout", "15", "--output", "/dev/null", "--range", "0-0", \ + "--write-out", "%{http_code}", url + secure_url = url.sub "http", "https" + secure_status_code, = curl_output "--connect-timeout", "15", "--output", "/dev/null", "--range", "0-0", \ + "--write-out", "%{http_code}", secure_url + if status_code.start_with?("20") && secure_status_code.start_with?("20") + problem "The URL #{url} could use HTTPS rather than HTTP" + end + end end def problem(text) From d4aa98d2300c0b81dd0d1608cf7fc67dbe5a6c04 Mon Sep 17 00:00:00 2001 From: David Broder-Rodgers Date: Mon, 30 Jan 2017 20:46:41 +0000 Subject: [PATCH 2/4] Updated mirror checks to compare ETags, Content-Lengths and binary files --- Library/Homebrew/dev-cmd/audit.rb | 34 ++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index af1e4a71bf..75e81db906 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -1481,18 +1481,42 @@ class ResourceAuditor urls.each do |url| next unless url.start_with? "http:" # Check for insecure mirrors - status_code, = curl_output "--connect-timeout", "15", "--output", "/dev/null", "--range", "0-0", \ - "--write-out", "%{http_code}", url + details = get_content_details(url, 1) secure_url = url.sub "http", "https" - secure_status_code, = curl_output "--connect-timeout", "15", "--output", "/dev/null", "--range", "0-0", \ - "--write-out", "%{http_code}", secure_url - if status_code.start_with?("20") && secure_status_code.start_with?("20") + secure_details = get_content_details(secure_url, 2) + + next unless details[:status].start_with?("2") && secure_details[:status].start_with?("2") + + if (details[:etag] && details[:etag] == secure_details[:etag]) \ + || (details[:content_length] && details[:content_length] == secure_details[:content_length]) \ + || are_same_file(details[:filename], secure_details[:filename]) problem "The URL #{url} could use HTTPS rather than HTTP" end + + remove_files details[:filename], secure_details[:filename] end end def problem(text) @problems << text end + + def get_content_details(url, id) + out = {} + out_file = "/tmp/_c#{id}" + headers, = curl_output "--connect-timeout", "15", "--output", out_file, "--dump-header", "/dev/stdout", url + out[:status] = headers[%r{HTTP\/.* (\d+)}, 1] + out[:etag] = headers[%r{ETag: ([wW]\/)?"(([^"]|\\")*)"}, 2] + out[:content_length] = headers[/Content-Length: (\d+)/, 1] + out[:filename] = out_file + out + end + + def are_same_file(one, two) + quiet_system "diff", "--report-identical-files", "--binary", "--speed-large-files", one, two + end + + def remove_files(*files) + quiet_system "rm", "-f", *files + end end From 64c83f3286a40cbe8871124f208fb3eaaaffd97a Mon Sep 17 00:00:00 2001 From: David Broder-Rodgers Date: Thu, 2 Feb 2017 21:25:29 +0000 Subject: [PATCH 3/4] Use file checksum rather than file diffing --- Library/Homebrew/dev-cmd/audit.rb | 44 ++++++++++++++----------------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 75e81db906..b4906cb80a 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -36,6 +36,7 @@ require "cmd/search" require "cmd/style" require "date" require "blacklist" +require "digest" module Homebrew module_function @@ -1479,21 +1480,23 @@ class ResourceAuditor return unless @online urls.each do |url| - next unless url.start_with? "http:" - # Check for insecure mirrors - details = get_content_details(url, 1) - secure_url = url.sub "http", "https" - secure_details = get_content_details(secure_url, 2) + check_insecure_mirror(url) if url.start_with? "http:" + end + end - next unless details[:status].start_with?("2") && secure_details[:status].start_with?("2") + def check_insecure_mirror(url) + details = get_content_details(url) + secure_url = url.sub "http", "https" + secure_details = get_content_details(secure_url) - if (details[:etag] && details[:etag] == secure_details[:etag]) \ - || (details[:content_length] && details[:content_length] == secure_details[:content_length]) \ - || are_same_file(details[:filename], secure_details[:filename]) - problem "The URL #{url} could use HTTPS rather than HTTP" - end + return if !details[:status].start_with?("2") || !secure_details[:status].start_with?("2") - remove_files details[:filename], secure_details[:filename] + etag_match = details[:etag] && details[:etag] == secure_details[:etag] + content_length_match = details[:content_length] && details[:content_length] == secure_details[:content_length] + file_match = details[:file_hash] == secure_details[:file_hash] + + if etag_match || content_length_match || file_match + problem "The URL #{url} could use HTTPS rather than HTTP" end end @@ -1501,22 +1504,15 @@ class ResourceAuditor @problems << text end - def get_content_details(url, id) + def get_content_details(url) out = {} - out_file = "/tmp/_c#{id}" - headers, = curl_output "--connect-timeout", "15", "--output", out_file, "--dump-header", "/dev/stdout", url + output, = curl_output "--connect-timeout", "15", "--include", url + split = output.partition("\r\n\r\n") + headers = split.first out[:status] = headers[%r{HTTP\/.* (\d+)}, 1] out[:etag] = headers[%r{ETag: ([wW]\/)?"(([^"]|\\")*)"}, 2] out[:content_length] = headers[/Content-Length: (\d+)/, 1] - out[:filename] = out_file + out[:file_hash] = Digest::SHA256.digest split.last out end - - def are_same_file(one, two) - quiet_system "diff", "--report-identical-files", "--binary", "--speed-large-files", one, two - end - - def remove_files(*files) - quiet_system "rm", "-f", *files - end end From b2dd6bc9b0c765104898d128c455b2f107498399 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Wed, 15 Feb 2017 14:41:06 +0000 Subject: [PATCH 4/4] audit: fix brew style warning. --- Library/Homebrew/dev-cmd/audit.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index b4906cb80a..5e674cf125 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -1495,9 +1495,8 @@ class ResourceAuditor content_length_match = details[:content_length] && details[:content_length] == secure_details[:content_length] file_match = details[:file_hash] == secure_details[:file_hash] - if etag_match || content_length_match || file_match - problem "The URL #{url} could use HTTPS rather than HTTP" - end + return if !etag_match && !content_length_match && !file_match + problem "The URL #{url} could use HTTPS rather than HTTP" end def problem(text)