546536 - Several small fixes to weave sync input validation

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Michael Hanson]

In the course of getting all the corner-case unit tests to pass, I submit the following edits:

--- 1.0/weave_basic_object.php  2010-02-16 14:19:30.000000000 -0800
+++ ../ut-fixes/1.0/weave_basic_object.php   2010-02-16 15:35:20.000000000 -0800
@@ -75,6 +75,12 @@ class wbo
 
                if (array_key_exists('sortindex', $extracted))
                {
+                       # Due to complicated logic in the getter, we need to validate
+                       # the value space of sortindex here.
+                       if (!is_numeric($extracted['sortindex'])) {
+                               $this->_error[] = "invalid sortindex";
+                               return 0;
+                       }
                        $this->sortindex($extracted['sortindex']);
                }
                
@@ -185,7 +193,8 @@ class wbo
        function validate()
        {
                
-               if (!$this->id() || strlen($this->id()) > 64 || strpos($this->id(), '/') !== false)
+               if (!$this->id() || strlen($this->id()) > 64  || strpos($this->id(), '/') !== false
+                       || strpos($this->id(), '#') !== false || strpos($this->id(), '?') !== false)
                { $this->_error[] = "invalid id"; }
 
                if ($this->parentid_exists() && strlen($this->parentid()) > 64)
@@ -203,7 +212,10 @@ class wbo
                if (!$this->_collection || strlen($this->_collection) > 64)
                { $this->_error[] = "invalid collection"; }
                
-               if ($this->sortindex_exists() && !is_numeric($this->sortindex()))
+               if ($this->sortindex_exists() && 
+                    (!is_numeric($this->wbo_hash['sortindex']) || 
+                     intval($this->sortindex()) > 999999999 || 
+                     intval($this->sortindex()) < -999999999 ))
                { $this->_error[] = "invalid sortindex"; }
                
                if ($this->payload_exists())
--- 1.0/weave_storage/mysql.php 2010-02-16 14:19:30.000000000 -0800
+++ ../2010-02-16-checkin-weaveserver-sync/1.0/weave_storage/mysql.php  2010-02-16 15:35:20.000000000 -0800
@@ -643,13 +643,24 @@ class WeaveStorage implements WeaveStora
                        $select_stmt .= " order by modified";
                }
                
+
                if ($limit)
                {
-                       $select_stmt .= " limit " . intval($limit);
+                       $limitVal = intval($limit);
+                       if ($limitVal < 0) {
+                               throw new Exception("Illegal limit value", 400);
+                       }
+                       $select_stmt .= " limit " . $limitVal;
                        if ($offset)
                        {
-                               $select_stmt .= " offset " . intval($offset);
+                               $offsetVal = intval($offset);
+                               if ($offsetVal < 0) {
+                                       throw new Exception("Illegal offset value", 400);
+                               }
+                               $select_stmt .= " offset " . $offsetVal;
                        }
+               } else if ($offset) {
+                       throw new Exception("Offset requires limit", 400); 
                }
 
                try
@@ -774,11 +785,21 @@ class WeaveStorage implements WeaveStora
                
                if ($limit)
                {
-                       $select_stmt .= " limit " . intval($limit);
+                       $limitVal = intval($limit);
+                       if ($limitVal < 0) {
+                               throw new Exception("Illegal limit value", 400);
+                       }
+                       $select_stmt .= " limit " . $limitVal;
                        if ($offset)
                        {
-                               $select_stmt .= " offset " . intval($offset);
+                               $offsetVal = intval($offset);
+                               if ($offsetVal < 0) {
+                                       throw new Exception("Illegal offset value", 400);
+                               }
+                               $select_stmt .= " offset " . $offsetVal;
                        }
+               } else if ($offset) {
+                       throw new Exception("Offset requires limit", 400);              
                }
                
                try

Comment 1

[Toby Elliott [:telliott]]

These are great, but I'm adding Ed here, since I'm concerned about the additional characters. I agree that they are URL problematic (in a way, I'd like to kill < and > as well) and can't be gotten directly, but we should be a little careful that they can't be gotten as a group.

Comment 2

[Ed Lee :Mardak]

What do you mean by kill < and > ?

Also, what's the idea behind checking for "#" and "?". If the URI has an unescaped #, the server won't be seeing it anyway as it's for the client to handle. For example, if I need to curl GET a GUID with {, I have to enter it as %3b, but on the server side, I would assume this->id() to have the actual character "{".

For the Weave add-on generated GUIDs, it pulls from these characters:
"!()*-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz~"

Part of the reason for those characters in particular is that they don't change on %encoding.

Comment 3

[Toby Elliott [:telliott]]

(In reply to comment #2)
> What do you mean by kill < and > ?

Disallow them as valid id characters. It's way off scope for the application, but it might save frontend developers some security headaches later.



> Also, what's the idea behind checking for "#" and "?". If the URI has an
> unescaped #, the server won't be seeing it anyway as it's for the client to
> handle. For example, if I need to curl GET a GUID with {, I have to enter it as
> %3b, but on the server side, I would assume this->id() to have the actual
> character "{".

I could, in theory, POST an item with an ID of "#?". It wouldn't be accessible directly, but would be returned in a modified-since query. The question is whether to prevent these three characters (and possibly <>) from ever being input into the system.



> 
> For the Weave add-on generated GUIDs, it pulls from these characters:
> "!()*-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz~"
> 
> Part of the reason for those characters in particular is that they don't change
> on %encoding.

Can we have the client strip them out of places? Is the above list of characters actually sufficient? That would be great.

Comment 4

[Toby Elliott [:telliott]]

live in the 2/25/10 push. We only disallow '/' from the ids at this time