add missing cvss_v3 scores during github sync #479

skipkayhil · 2021-06-19T22:31:37Z

I came across some vulnerabilities that were missing cvss scores present on Github, so I wanted to update the github sync script to pull down those values automatically. I broke the change into three commits:

The first commit is a refactor of the github sync to query vulnerabilities instead of advisories. This was done because I noticed that the number of queries made to pull down all of the vulnerabilities seemed too high (43 when I first ran it), and it turned out that it was getting advisories for all ecosystems and not just ruby. After the refactor, it is able to pull all of the ruby vulnerabilities and advisories in 7 requests.
The second commit is the change that adds missing cvss scores if the score is present on Github.
The third is the result of running the script, adding 25 missing cvss scores

Let me know if you have any feedback or if there is anything I should do differently that could help this get merged!

postmodern · 2021-06-19T23:27:39Z

@reedloden or @rschultheis could you review the changes to the github sync script, there's a lot of changes.

phillmv

I like it. I read over the code, and it seems largely benign. I left a variety of comments, mostly minor style observations.

My recommendation is thus:

Can you split off the updates to the data into a 2nd, separate, PR so that we can look only at the changes to the lib/github_advisory_sync? Then once this is approved, we can just rubber-stamp the data update.

phillmv · 2021-06-21T14:25:26Z

lib/github_advisory_sync.rb

-        raise(GitHubGraphQLAPIError, body_obj["errors"].map { |e| e["message"] }.join(", "))
-      end
+
+      raise(GitHubGraphQLAPIError, body_obj["errors"].map { |e| e["message"] }.join(", ")) if body_obj["errors"]


minor style issue:

for my money, better to have

if error raise end

than raise foo if error, this raise line is almost trivial enough that foo if errors is OK but not quite trivial enough for my money ;)

lib/github_advisory_sync.rb

phillmv · 2021-06-21T14:30:53Z

lib/github_advisory_sync.rb

+
+        next if advisory.withdrawn?
+
+        advisories[advisory.primary_id] = advisory unless advisories.key? advisory.primary_id


i believe this will do just fine too 😄

Suggested change

advisories[advisory.primary_id] = advisory unless advisories.key? advisory.primary_id

advisories[advisory.primary_id] ||= advisory

phillmv · 2021-06-21T14:34:47Z

lib/github_advisory_sync.rb

        query($first: Int, $after: String) {
-          securityAdvisories(first: $first, after: $after) {
+          securityVulnerabilities(first: $first, after: $after, ecosystem:RUBYGEMS) {


Very good! We'd recently moved ecosystem down to the vulnerability node, and I'm glad you noticed this here.

Seeing this tho, I wonder two things @skipkayhil:

life would be easier if offered an ecosystem filter on securityAdvisories, eh?

would it be useful to provide an updated_after filter, so we don't have to page thru every single advisory? that way maybe we could just store a timestamp of the last time the importer was run.

Both of those sounds like great additions! I think adding ecosystem to securityAdvisories would be a huge win for sure, and probably remove a lot of the refactoring I did to invert the relationship

We're probably going to touch the API this summer, so I'll add this to my list of tweaks. If you have any other suggestions, holler.

phillmv · 2021-06-21T14:37:24Z

lib/github_advisory_sync.rb

    end

    def external_reference
-      github_advisory_graphql_object["references"].find do |ref|
+      advisory["references"].each do |ref|


this is fine but i would have left find in place.

also of note is iirc we often include many different references now in the data, so looking for the first non-nvd url might not always be what we want 🤔

phillmv · 2021-06-21T14:38:49Z

lib/github_advisory_sync.rb

-      gh_advisories.each do |advisory|
-        files_written += advisory.write_files
-      end
+      files_written = gh_advisories.filter_map(&:write_file)


i approve of this main loop simplification!

lib/github_advisory_sync.rb

phillmv · 2021-06-21T14:41:25Z

lib/github_advisory_sync.rb

-    def vulnerabilities
-      github_advisory_graphql_object["vulnerabilities"]["nodes"]
+    def package_name
+      vulnerabilities.first["package"]["name"]


in practice, this is fine but of note is that it's technically possible for one advisory to span many different package names.

Do we expect that to ever happen? If so, we should probably support that... See also #400

Updated to allow for multiple package names

@reedloden the schema changes enabling that shipped the same time we made this change #479 (comment)

i think in practice it's just extremely rare 🤔 might not have happened yet in ruby land, i'd have to poke the API to find out :P.

phillmv · 2021-06-21T14:43:19Z

lib/github_advisory_sync.rb

-          file.write "\n\n# GitHub advisory data below - **Remove this data before committing**\n"
-          file.write "# Use this data to write patched_versions (and potentially unaffected_versions) above\n"
-          file.write github_advisory_graphql_object.to_yaml
+      return if saved_data.key?("cvss_v3") || cvss.nil?


if we're only checking to update cvss_v3, maybe we can rename the method accordingly, update_file_cvss_v3 or something.

Can we support updating other things? Would be nice to have an (optional) way to sync more than just CVSSv3, as if the advisories are updated, we might not pick up that at all.

Updated to add any missing information from Github

phillmv · 2021-06-21T14:44:39Z

lib/github_advisory_sync.rb

+      cvss_added = false
+      File.open("#{rubysec_filename}.tmp", "w") do |f| 
+        IO.foreach(rubysec_filename) do |line|
+          if (line.include?('unaffected_versions:') || line.include?('patched_versions:')) && !cvss_added


This gives me pause. Why not just add the cvss_v3 key to the saved_data, and re-emit the YAML wholesale? Is the diff too ugly otherwise?

I agree, this is pretty ugly. The issue with YAML.load/dump was that it wasn't dumping multi line strings for me, the new file would look like this:

--- gem: actionpack framework: rails cve: 2015-7581 date: 2016-01-25 url: https://groups.google.com/forum/#!topic/rubyonrails-security/dthJ5wL69JE title: Object leak vulnerability for wildcard controller routes in Action Pack description: "There is an object leak vulnerability for wildcard controllers in Action Pack. \nThis vulnerability has been assigned the CVE identifier CVE-2015-7581. \n\nVersions Affected: >= 4.0.0 and < 5.0.0.beta1 \nNot affected: < 4.0.0, 5.0.0.beta1 and newer \nFixed Versions: 4.2.5.1, 4.1.14.1 \n\nImpact \n------ \nUsers that have a route that contains the string \":controller\" are susceptible \nto objects being leaked globally which can lead to unbounded memory growth. \nTo identify if your application is vulnerable, look for routes that contain \n\":controller\". \n\nInternally, Action Pack keeps a map of \"url controller name\" to \"controller \nclass name\". This map is cached globally, and is populated even if the \ncontroller class doesn't actually exist. \n\nAll users running an affected release should either upgrade or use one of the \nworkarounds immediately. \n\nReleases \n-------- \nThe FIXED releases are available at the normal locations. \n\nWorkarounds \n----------- \nThere are no feasible workarounds for this issue. \n\nPatches \n------- \nTo aid users who aren't able to upgrade immediately we have provided patches for the two supported release series. They are in git-am format and consist of a single changeset. \n\n* 4-1-wildcard_route.patch - Patch for 4.1 series \n* 4-2-wildcard_route.patch - Patch for 4.2 series \n\nPlease note that only the 4.1.x and 4.2.x series are supported at present. Users of earlier unsupported releases are advised to upgrade as soon as possible as we cannot guarantee the continued availability of security fixes for unsupported releases.\n" unaffected_versions: - "< 4.0.0" - ">= 5.0.0.beta1" patched_versions: - "~> 4.2.5, >= 4.2.5.1" - "~> 4.1.14, >= 4.1.14.1" cvss_v3: 7.5

If I'm missing some parameter that would make it keep the "pretty" multi line strings then that would be way better than what I tried

Edit:
I found an issue on Psych that looks related: ruby/psych#364
I'll dig into this some more and see what I can find

I figured it out, YAML.dump is trying to preserve the trailing whitespace in some of the files, causing the weird output seen above.

I opened another PR to remove all of the trailing whitespace from the yaml files: #481
Maybe a linting Github Action could be added to prevent any occurrences in the future?

I'll update this PR to use load/dump tomorrow

reedloden

Looking good. Looks like you have a few more things you want to change, and I have some random comments. But overall, excellent changes. Thanks again!

reedloden · 2021-06-22T02:01:26Z

lib/github_advisory_sync.rb

-    def vulnerabilities
-      github_advisory_graphql_object["vulnerabilities"]["nodes"]
+    def package_name
+      vulnerabilities.first["package"]["name"]


Do we expect that to ever happen? If so, we should probably support that... See also #400

reedloden · 2021-06-22T02:02:42Z

lib/github_advisory_sync.rb

-          file.write "\n\n# GitHub advisory data below - **Remove this data before committing**\n"
-          file.write "# Use this data to write patched_versions (and potentially unaffected_versions) above\n"
-          file.write github_advisory_graphql_object.to_yaml
+      return if saved_data.key?("cvss_v3") || cvss.nil?


Can we support updating other things? Would be nice to have an (optional) way to sync more than just CVSSv3, as if the advisories are updated, we might not pick up that at all.

reedloden · 2021-06-22T02:04:24Z

lib/github_advisory_sync.rb

+        "unaffected_versions" => ["<OPTIONAL: FILL IN SEE BELOW>"]
+      }
+      data["cve"] = cve_id[4..20] if cve_id
+      data["cvss_v3"] = "<FILL IN IF AVAILABLE>" if cvss.nil?


Is this where we should at some future point use NVD's API to check if they have an available score? cough #399

reedloden · 2021-06-22T02:04:38Z

lib/github_advisory_sync.rb

+        # - GitHub has a first_patched_version field,
+        #   but rubysec advisory needs a ruby version spec
+        #
+        # The unnaffected_versions field is similarly not directly available


unaffected*

reedloden · 2021-06-22T02:53:33Z

lib/github_advisory_sync.rb

-        next if ref["url"].start_with?("https://nvd.nist.gov/vuln/detail/")
-        ref["url"]
+      advisory["references"].find do |ref|
+        !ref["url"].start_with?("https://nvd.nist.gov/vuln/detail/")


Eventually, would love to pick the first one as the main URL and add the others as related URLs at the bottom. #402

skipkayhil · 2021-06-23T03:47:08Z

Thanks for all the feedback! I've updated the PR to handle multiple package names in an advisory (in case that comes up), and it will now update any field from Github that is missing in the file.

After running the script, there are 167 files updated (I'll make another PR for that after this one is resolved). I've attached an example diff from gems/actionpack/CVE-2015-7576.yml. The only content change I see is the addition of the ghsa, and the rest is key ordering or formatting differences caused by YAML.dump. I tried changing the indentation option of YAML.dump but it did not change the indentation of arrays (patched/unpatched)

diff --git a/gems/actionpack/CVE-2015-7576.yml b/gems/actionpack/CVE-2015-7576.yml
index 300ddec..b5dbdfa 100644
--- a/gems/actionpack/CVE-2015-7576.yml
+++ b/gems/actionpack/CVE-2015-7576.yml
@@ -2,118 +2,115 @@
 gem: actionpack
 framework: rails
 cve: 2015-7576
-date: 2016-01-25
-url: "https://groups.google.com/forum/#!topic/rubyonrails-security/ANv0HDHEC3k"
-
+ghsa: p692-7mm3-3fxg
+url: https://groups.google.com/forum/#!topic/rubyonrails-security/ANv0HDHEC3k
 title: Timing attack vulnerability in basic authentication in Action Controller.
-
+date: 2016-01-25
 description: |
-    There is a timing attack vulnerability in the basic authentication support
-    in Action Controller. This vulnerability has been assigned the CVE
-    identifier CVE-2015-7576.
+  There is a timing attack vulnerability in the basic authentication support
+  in Action Controller. This vulnerability has been assigned the CVE
+  identifier CVE-2015-7576.

-    Versions Affected:  All.
-    Not affected:       None.
-    Fixed Versions:     5.0.0.beta1.1, 4.2.5.1, 4.1.14.1, 3.2.22.1
+  Versions Affected:  All.
+  Not affected:       None.
+  Fixed Versions:     5.0.0.beta1.1, 4.2.5.1, 4.1.14.1, 3.2.22.1

-    Impact
-    ------
-    Due to the way that Action Controller compares user names and passwords in
-    basic authentication authorization code, it is possible for an attacker to
-    analyze the time taken by a response and intuit the password.
+  Impact
+  ------
+  Due to the way that Action Controller compares user names and passwords in
+  basic authentication authorization code, it is possible for an attacker to
+  analyze the time taken by a response and intuit the password.

-    For example, this string comparison:
+  For example, this string comparison:

-      "foo" == "bar"
+    "foo" == "bar"

-    is possibly faster than this comparison:
+  is possibly faster than this comparison:

-      "foo" == "fo1"
+    "foo" == "fo1"

-    Attackers can use this information to attempt to guess the username and
-    password used in the basic authentication system.
+  Attackers can use this information to attempt to guess the username and
+  password used in the basic authentication system.

-    You can tell you application is vulnerable to this attack by looking for
-    `http_basic_authenticate_with` method calls in your application.
+  You can tell you application is vulnerable to this attack by looking for
+  `http_basic_authenticate_with` method calls in your application.

-    All users running an affected release should either upgrade or use one of
-    the workarounds immediately.
+  All users running an affected release should either upgrade or use one of
+  the workarounds immediately.

-    Releases
-    --------
-    The FIXED releases are available at the normal locations.
+  Releases
+  --------
+  The FIXED releases are available at the normal locations.

-    Workarounds
-    -----------
-    If you can't upgrade, please use the following monkey patch in an initializer
-    that is loaded before your application:
+  Workarounds
+  -----------
+  If you can't upgrade, please use the following monkey patch in an initializer
+  that is loaded before your application:

-    ```
-    $ cat config/initializers/basic_auth_fix.rb
-    module ActiveSupport
-      module SecurityUtils
-        def secure_compare(a, b)
-          return false unless a.bytesize == b.bytesize
+  ```
+  $ cat config/initializers/basic_auth_fix.rb
+  module ActiveSupport
+    module SecurityUtils
+      def secure_compare(a, b)
+        return false unless a.bytesize == b.bytesize

-          l = a.unpack "C#{a.bytesize}"
+        l = a.unpack "C#{a.bytesize}"

-          res = 0
-          b.each_byte { |byte| res |= byte ^ l.shift }
-          res == 0
-        end
-        module_function :secure_compare
+        res = 0
+        b.each_byte { |byte| res |= byte ^ l.shift }
+        res == 0
+      end
+      module_function :secure_compare

-        def variable_size_secure_compare(a, b)
-          secure_compare(::Digest::SHA256.hexdigest(a), ::Digest::SHA256.hexdigest(b))
-        end
-        module_function :variable_size_secure_compare
+      def variable_size_secure_compare(a, b)
+        secure_compare(::Digest::SHA256.hexdigest(a), ::Digest::SHA256.hexdigest(b))
       end
+      module_function :variable_size_secure_compare
     end
-
-    module ActionController
-      class Base
-        def self.http_basic_authenticate_with(options = {})
-          before_action(options.except(:name, :password, :realm)) do
-            authenticate_or_request_with_http_basic(options[:realm] || "Application") do |name, password|
-              # This comparison uses & so that it doesn't short circuit and
-              # uses `variable_size_secure_compare` so that length information
-              # isn't leaked.
-              ActiveSupport::SecurityUtils.variable_size_secure_compare(name, options[:name]) &
-                ActiveSupport::SecurityUtils.variable_size_secure_compare(password, options[:password])
-            end
+  end
+
+  module ActionController
+    class Base
+      def self.http_basic_authenticate_with(options = {})
+        before_action(options.except(:name, :password, :realm)) do
+          authenticate_or_request_with_http_basic(options[:realm] || "Application") do |name, password|
+            # This comparison uses & so that it doesn't short circuit and
+            # uses `variable_size_secure_compare` so that length information
+            # isn't leaked.
+            ActiveSupport::SecurityUtils.variable_size_secure_compare(name, options[:name]) &
+              ActiveSupport::SecurityUtils.variable_size_secure_compare(password, options[:password])
           end
         end
       end
     end
-    ```
+  end
+  ```


-    Patches
-    -------
-    To aid users who aren't able to upgrade immediately we have provided patches for
-    the two supported release series. They are in git-am format and consist of a
-    single changeset.
+  Patches
+  -------
+  To aid users who aren't able to upgrade immediately we have provided patches for
+  the two supported release series. They are in git-am format and consist of a
+  single changeset.

-    * 4-1-basic_auth.patch - Patch for 4.1 series
-    * 4-2-basic_auth.patch - Patch for 4.2 series
-    * 5-0-basic_auth.patch - Patch for 5.0 series
+  * 4-1-basic_auth.patch - Patch for 4.1 series
+  * 4-2-basic_auth.patch - Patch for 4.2 series
+  * 5-0-basic_auth.patch - Patch for 5.0 series

-    Please note that only the 4.1.x and 4.2.x series are supported at present. Users
-    of earlier unsupported releases are advised to upgrade as soon as possible as we
-    cannot guarantee the continued availability of security fixes for unsupported
-    releases.
+  Please note that only the 4.1.x and 4.2.x series are supported at present. Users
+  of earlier unsupported releases are advised to upgrade as soon as possible as we
+  cannot guarantee the continued availability of security fixes for unsupported
+  releases.

-    Credits
-    -------
-
-    Thank you to Daniel Waterworth for reporting the problem and working with us to
-    fix it.
+  Credits
+  -------

+  Thank you to Daniel Waterworth for reporting the problem and working with us to
+  fix it.
 cvss_v2: 4.3
 cvss_v3: 3.7
-
 patched_versions:
-  - ">= 5.0.0.beta1.1"
-  - "~> 4.2.5, >= 4.2.5.1"
-  - "~> 4.1.14, >= 4.1.14.1"
-  - "~> 3.2.22.1"
+- ">= 5.0.0.beta1.1"
+- "~> 4.2.5, >= 4.2.5.1"
+- "~> 4.1.14, >= 4.1.14.1"
+- "~> 3.2.22.1"

Instead of querying all Github Advisories and then filtering out the advisories that have no ruby vulnerabilities, this change makes the query target ruby vulnerabilities directly. Before, running the script made 43 requests (for a total of 4257 advisories). After, it makes 7 requests (for a total of 665 vulnerabilities).

phillmv · 2021-06-24T19:58:43Z

My eyes are too tired to give this justice right now, I'll circle back on this tomorrow. @skipkayhil if the YAML.load(old) == YAML.load(new).except('ghsa') matches, that's good enough for me.

phillmv

This seems fine to me! Thanks for the update 😄. Ideally we'd have tests 🤔 but if it works that's good enough for now.

postmodern requested a review from reedloden June 19, 2021 23:26

phillmv reviewed Jun 21, 2021

View reviewed changes

skipkayhil mentioned this pull request Jun 21, 2021

update vulnerabilities with cvss_v3 from github #480

Merged

skipkayhil force-pushed the cvss-updater branch from b36b13d to f3415e7 Compare June 21, 2021 19:51

skipkayhil mentioned this pull request Jun 22, 2021

remove trailing whitespaces from yml files #481

Merged

reedloden reviewed Jun 22, 2021

View reviewed changes

skipkayhil force-pushed the cvss-updater branch 2 times, most recently from 36ede4c to da536ac Compare June 23, 2021 03:31

skipkayhil added 2 commits June 22, 2021 23:49

add any missing data during github sync

5eef30d

skipkayhil force-pushed the cvss-updater branch from da536ac to 5eef30d Compare June 23, 2021 03:50

reedloden requested a review from phillmv June 24, 2021 15:29

phillmv approved these changes Jun 25, 2021

View reviewed changes

phillmv merged commit b562149 into rubysec:master Jun 25, 2021

skipkayhil deleted the cvss-updater branch September 1, 2021 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add missing cvss_v3 scores during github sync #479

add missing cvss_v3 scores during github sync #479

skipkayhil commented Jun 19, 2021

postmodern commented Jun 19, 2021

phillmv left a comment •

edited

Loading

phillmv Jun 21, 2021

phillmv Jun 21, 2021

phillmv Jun 21, 2021

skipkayhil Jun 21, 2021

phillmv Jun 21, 2021

phillmv Jun 21, 2021

phillmv Jun 21, 2021

phillmv Jun 21, 2021

phillmv Jun 21, 2021

reedloden Jun 22, 2021

skipkayhil Jun 23, 2021

phillmv Jun 24, 2021

phillmv Jun 21, 2021

reedloden Jun 22, 2021

skipkayhil Jun 23, 2021

phillmv Jun 21, 2021

skipkayhil Jun 21, 2021 •

edited

Loading

skipkayhil Jun 22, 2021

reedloden left a comment

reedloden Jun 22, 2021

reedloden Jun 22, 2021

reedloden Jun 22, 2021

reedloden Jun 22, 2021

reedloden Jun 22, 2021

skipkayhil commented Jun 23, 2021

phillmv commented Jun 24, 2021

phillmv left a comment


		next if advisory.withdrawn?

		advisories[advisory.primary_id] = advisory unless advisories.key? advisory.primary_id

	advisories[advisory.primary_id] = advisory unless advisories.key? advisory.primary_id
	advisories[advisory.primary_id] \|\|= advisory

add missing cvss_v3 scores during github sync #479

add missing cvss_v3 scores during github sync #479

Conversation

skipkayhil commented Jun 19, 2021

postmodern commented Jun 19, 2021

phillmv left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipkayhil Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedloden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipkayhil commented Jun 23, 2021

phillmv commented Jun 24, 2021

phillmv left a comment

Choose a reason for hiding this comment

phillmv left a comment •

edited

Loading

skipkayhil Jun 21, 2021 •

edited

Loading