Opened 5 years ago

Closed 5 years ago

#2589 closed defect (fixed)

Waterfall help content spoofing

Reported by: wms Owned by: Mikhail Sobolev <sa2ajj@…>
Priority: patches-accepted Milestone: 0.8.x
Version: 0.8.8 Keywords:
Cc:

Description

It seems like there is a problem with content spoofing on the waterfall help page. If you load the following URL:

http://buildbot.buildbot.net/waterfall/help?reload=XXXXX

You'll see that one of the reload options is now XXXXX, and can be whatever else I put there.

Change History (11)

comment:1 Changed 5 years ago by dustin

True, but it's properly escaped, so it can't be used for XSS or anything like that. Why is this a problem?

comment:2 Changed 5 years ago by wms

I believe the concern is that you can put arbitrary text in there for social engineering attacks such as "you need to reset your password, contact <malicious phone or email>".

comment:3 Changed 5 years ago by dustin

  • Milestone changed from undecided to 0.8.x

It's not a bad bugfix for the 'eight' branch.

comment:4 Changed 5 years ago by wms

Similar problem with the builder page's numbuilds parameter: https://build.webkit.org/builders/WinCairo%20Release?numbuilds=XXXX

comment:5 Changed 5 years ago by wms

Here's a patch against 0.8.9 for both the waterfall help and several uses of int() around the numbuilds param. I feel like there's probably a better fix by wrapping all uses of request.args.get() in a function that does some type checking and sanitizing, but this fixes the things mentioned here.

diff -ru buildbot.orig/status/web/builder.py buildbot/status/web/builder.py
--- buildbot.orig/status/web/builder.py	2014-10-02 14:28:53.000000000 -0700
+++ buildbot/status/web/builder.py	2014-10-02 14:42:08.000000000 -0700
@@ -362,7 +362,10 @@
                 'properties': properties,
             })
 
-        numbuilds = cxt['numbuilds'] = int(req.args.get('numbuilds', [self.numbuilds])[0])
+        try:
+            numbuilds = cxt['numbuilds'] = int(req.args.get('numbuilds', [self.numbuilds])[0])
+        except:
+            numbuilds = cxt['numbuilds'] = 10
         maxsearch = int(req.args.get('maxsearch', [200])[0])
         recent = cxt['recent'] = []
         for build in b.generateFinishedBuilds(
diff -ru buildbot.orig/status/web/olpb.py buildbot/status/web/olpb.py
--- buildbot.orig/status/web/olpb.py	2014-10-02 14:28:53.000000000 -0700
+++ buildbot/status/web/olpb.py	2014-10-02 14:42:26.000000000 -0700
@@ -55,7 +55,10 @@
 
     def content(self, req, cxt):
         status = self.getStatus(req)
-        numbuilds = int(req.args.get("numbuilds", [self.numbuilds])[0])
+        try:
+            numbuilds = int(req.args.get("numbuilds", [self.numbuilds])[0])
+        except:
+            numbuilds = 10
         builders = req.args.get("builder", [])
         branches = [b for b in req.args.get("branch", []) if b]
 
@@ -104,7 +107,10 @@
         self.pageTitle = "Recent Builds of %s" % self.builder_name
 
     def content(self, req, cxt):
-        numbuilds = int(req.args.get("numbuilds", [self.numbuilds])[0])
+        try:
+            numbuilds = int(req.args.get("numbuilds", [self.numbuilds])[0])
+        except:
+            numbuilds = 10
         branches = [b for b in req.args.get("branch", []) if b]
 
         # walk backwards through all builds of a single builder
diff -ru buildbot.orig/status/web/status_json.py buildbot/status/web/status_json.py
--- buildbot.orig/status/web/status_json.py	2014-10-02 14:28:53.000000000 -0700
+++ buildbot/status/web/status_json.py	2014-10-02 14:43:17.000000000 -0700
@@ -650,7 +650,10 @@
             builds = []
             builder_status = self.status.getBuilder(builderName)
             cache_size = builder_status.master.config.caches['Builds']
-            numbuilds = int(request.args.get('numbuilds', [cache_size - 1])[0])
+            try:
+                numbuilds = int(request.args.get('numbuilds', [cache_size - 1])[0])
+            except:
+                numbuilds = 10
             for i in range(1, numbuilds):
                 build_status = builder_status.getBuild(-i)
                 if not build_status or not build_status.isFinished():
diff -ru buildbot.orig/status/web/waterfall.py buildbot/status/web/waterfall.py
--- buildbot.orig/status/web/waterfall.py	2014-10-02 14:28:53.000000000 -0700
+++ buildbot/status/web/waterfall.py	2014-10-02 14:45:55.000000000 -0700
@@ -333,6 +333,8 @@
         current_reload_time = request.args.get("reload", ["none"])
         if current_reload_time:
             current_reload_time = current_reload_time[0]
+        if not current_reload_time.isdigit():
+            current_reload_time = "none"
         if current_reload_time not in [t[0] for t in times]:
             times.insert(0, (current_reload_time, current_reload_time))

comment:6 Changed 5 years ago by sa2ajj

@wms, do you think you could submit a PR directly to https://github.com/buildbot/buildbot ?

comment:8 Changed 5 years ago by sa2ajj

  • Priority changed from major to patches-accepted

Looks good, please see the comments.

comment:9 Changed 5 years ago by William Siegrist <wsiegrist@…>

In d2c00b3fa4629387f23bd9661d6a395309a53d68:

Proposed patch for #2589 to guard against content spoofing.

comment:10 Changed 5 years ago by William Siegrist <wsiegrist@…>

In d044297cf888e827da0e9a9f953463e35cbfac6d:

Update release notes for #2589

comment:11 Changed 5 years ago by Mikhail Sobolev <sa2ajj@…>

  • Owner set to Mikhail Sobolev <sa2ajj@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 99874c2eacd8b613e48dd4e57f2a34e36795cecc:

Merge pull request #1244 from wmswms/eight

Guard against content spoofing.

Fixes ticket:2589

Note: See TracTickets for help on using tickets.