Do not HTML escape stuff we just got from the user before writing it to LDAP,

set it as passwords, etc.  Instead escape stuff we did read from LDAP.

diff --git a/debian/changelog b/debian/changelog
index 2c3169e..a88baf7 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+userdir-ldap-cgi (0.3.19) unstable; urgency=low
+
+  * Do not HTML escape stuff we just got from the user before
+    writing it to LDAP, set it as passwords, etc.  Instead
+    escape stuff we did read from LDAP.
+
+ -- Peter Palfrader <weasel@debian.org>  Tue, 16 Sep 2008 16:21:32 +0200
+
 userdir-ldap-cgi (0.3.18) unstable; urgency=low
 
   * Add password checking via a python wrapper.

diff --git a/search.cgi b/search.cgi
index ff99da3..e3eb882 100755 (executable)
--- a/search.cgi
+++ b/search.cgi
@@ -118,6 +118,9 @@ if (!$dosearch) {
   foreach $dn (sort {$entries->{$a}->{sn}->[0] <=> $entries->{$b}->{sn}->[0]} keys(%$entries)) {
     my $ok = 0;
     $data = $entries->{$dn};
+    for my $key (keys %{$data}) {
+      @{$data->{$key}} = map { CGI::escapeHTML($_); } @{$data->{$key}};
+    }
 
     # These are local variables.. i have enough global vars as it is... <sigh>
     my ($ufdn, $login, $name, $icquin, $jabberjid, $email, $fingerprint,

diff --git a/update.cgi b/update.cgi
index 716faa9..8e74b3e 100755 (executable)
--- a/update.cgi
+++ b/update.cgi
@@ -82,8 +82,9 @@ if (!($query->param('doupdate'))) {
   # First do the easy stuff - this catches most of the cases
   foreach (keys(%$entry)) {
     $data{$_} = $entry->{$_}->[0];
+    $data{$_} = CGI::escapeHTML($data{$_}) if defined $data{$_};
   }
-  
+
   $data{gender} = 9 if not $data{gender};
 
   # Now we have to fill in the rest that needs some processing...
@@ -92,6 +93,7 @@ if (!($query->param('doupdate'))) {
   $data{editdn} = $editdn;
   $data{staddress} = $entry->{postaladdress}->[0];
   $data{staddress} =~ s/\$/\n/;
+  $data{staddress} = CGI::escapeHTML($data{staddress});
   $data{countryname} = &Util::LookupCountry($data{c});
 
   if ($data{mailgreylisting} eq "TRUE") {
@@ -106,7 +108,7 @@ if (!($query->param('doupdate'))) {
     $data{mailcallout} = "";
   }
   
-  $data{email} = join(", ", @{$entry->{emailforward}});  
+  $data{email} = CGI::escapeHTML(join(", ", @{$entry->{emailforward}}));
 
   my $genderselect = '<select name="gender">'
                   . '<option value="9"'
@@ -126,16 +128,16 @@ if (!($query->param('doupdate'))) {
       next;
     }
     $status =~ s/:.*//; # remove verification hmac, it's just noise here.
-    my $e = "<tr><td>$hosts</td>
-                 <td>$status</td>
+    my $e = "<tr><td>".CGI::escapeHTML($hosts)."</td>
+                 <td>".CGI::escapeHTML($status)."</td>
                  <td><small>not shown</small></td>
-                 <!--<td><small><code>$uuid</code></small></td>-->
-                 <td><input name=\"sudopassword-delete-$uuid\" type=\"checkbox\" value=\"delete\"> (delete)</td></tr>\n";
+                 <!--<td><small><code>".CGI::escapeHTML($uuid)."</code></small></td>-->
+                 <td><input name=\"sudopassword-delete-".CGI::escapeHTML($uuid)."\" type=\"checkbox\" value=\"delete\"> (delete)</td></tr>\n";
     $sudopassword .= $e;
     if ($status eq 'unconfirmed') {
       my $data = join(':', 'confirm-new-password', $uuid, $hosts, $crypted);
       my $hmac = hmac_sha1_hex( $data, $hmac_key);
-      $confirmstring .= "confirm sudopassword $uuid $hosts $hmac\n";
+      $confirmstring .= CGI::escapeHTML("confirm sudopassword $uuid $hosts $hmac\n");
     }
   };
   if ($confirmstring ne '') {
@@ -176,7 +178,10 @@ if (!($query->param('doupdate'))) {
   # Actually update stuff...
   my ($newpassword, $newstaddress);
   
-  &Util::FixParams($query);
+  # Good god, why would we want to do that here?  it breaks password setting
+  # etc, and it doesn't prevent people from setting eveil stuff in ldap
+  # directly.
+  # &Util::FixParams($query);
 
   if (($query->param('labeleduri')) && 
       ($query->param('labeleduri') !~ /^https?:\/\//i)) {