A user.login or system.connect request via XMLRPC returns the logged in user's password hash. Is that correct behavior?

<methodResponse>
  <params>
  <param>
    <value><struct>
  <member><name>sessid</name><value><string>asNTVaZ2Fc5ba_QyYR4st8-dLp1lzHYwl0VdvuQKKWA</string></value></member>
  <member><name>user</name><value><struct>
  <member><name>uid</name><value><string>4</string></value></member>
  <member><name>name</name><value><string>player1</string></value></member>
  <member><name>pass</name><value><string>$S$Dw2Jn.uKv0kabe44kkT3C27uCOvvcLlgOcoAv4qKbk/oyE50JA32</string></value></member>
  <member><name>mail</name><value><string>player1@example.com</string></value></member>
  <member><name>theme</name><value><string></string></value></member>
  <member><name>signature</name><value><string></string></value></member>
  <member><name>signature_format</name><value><string>filtered_html</string></value></member>
  <member><name>created</name><value><string>1314911254</string></value></member>
  <member><name>access</name><value><string>1314917022</string></value></member>
  <member><name>login</name><value><string>1314917022</string></value></member>
  <member><name>status</name><value><string>1</string></value></member>
  <member><name>timezone</name><value><string>America/Chicago</string></value></member>
  <member><name>language</name><value><string></string></value></member>
  <member><name>picture</name><value><string>0</string></value></member>
  <member><name>init</name><value><string>player1@example.com</string></value></member>
  <member><name>data</name><value><boolean>0</boolean></value></member>
  <member><name>sid</name><value><string>asNTVaZ2Fc5ba_QyYR4st8-dLp1lzHYwl0VdvuQKKWA</string></value></member>
  <member><name>ssid</name><value><string></string></value></member>
  <member><name>hostname</name><value><string>127.0.0.1</string></value></member>
  <member><name>timestamp</name><value><string>1314917022</string></value></member>
  <member><name>cache</name><value><string>0</string></value></member>
  <member><name>session</name><value><string></string></value></member>
  <member><name>roles</name><value><struct>
  <member><name>2</name><value><string>authenticated user</string></value></member>
</struct></value></member>
</struct></value></member>
</struct></value>
  </param>
  </params>
</methodResponse>
CommentFileSizeAuthor
#12 0001-Issue-1267212-by-marcingy-Password-hash-leakage.patch2.51 KBygerasimov
#10 hash-leak-v3.patch2.08 KBmarcingy
#7 hash-leak-v2.patch2.08 KBmarcingy
#2 hash-leak.patch985 bytesmarcingy

Comments

kylebrowning’s picture

kylebrowning commented
Priority: Major » Critical

No its being removed.

marcingy’s picture

marcingy commented
Status: Active » Needs review
StatusFileSize
new hash-leak.patch985 bytes

Inital patch needs tests still just want to confirm that this is all areas that need changed

Status: Needs review » Needs work

The last submitted patch, hash-leak.patch, failed testing.

marcingy’s picture

marcingy commented

Bad test bot..your inability to enable ctools is sad :(

wedge’s picture

wedge commented

Shouldn't this be indented one more space?

+       // Remove the users password from the account object.
+       unset($result->pass);

And with the patch I still get hash leaks on user/login and system/connect.

marcingy’s picture

marcingy commented

I pretty well knew I was going to miss some areas.

I'll update the patch tonight to include those additional places and hopefully get the tests in place as well.

marcingy’s picture

marcingy commented
Version: 7.x-3.0-rc5 » 7.x-3.x-dev
Status: Needs work » Needs review
StatusFileSize
new hash-leak-v2.patch2.08 KB

Better version of the patch still needs tests.
* Strips data in
- system connect
- user index
- user update
- user login

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Needs work 
+++ b/services.moduleundefined
@@ -500,9 +500,23 @@ function services_resource_build_index_list($results, $type, $field) {
+      if ($type == 'user') {
+        services_remove_user_data(&$result)
+      }

This should be services_remove_user_data($result);

As it is not big change I don't think we should have separate test for this patch. I also looked through the code of resources and think that marcingy covered all cases we needed to sanitize the user object.

kylebrowning’s picture

kylebrowning commented

Looks good too me, I'm unsure on the pass by reference issue, was this tested on 5.3?

marcingy’s picture

marcingy commented
Status: Needs work » Needs review
StatusFileSize
new hash-leak-v3.patch2.08 KB

pass by reference is a bug and comes from a mindless cut and paste session :)

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Reviewed & tested by the community

looks good!

ygerasimov’s picture

ygerasimov commented
StatusFileSize
new 0001-Issue-1267212-by-marcingy-Password-hash-leakage.patch2.51 KB

Patch from #10 misses semicolon. Here is the same one. Please commit it.

kylebrowning’s picture

kylebrowning commented
Status: Reviewed & tested by the community » Patch (to be ported)
kylebrowning’s picture

kylebrowning commented
Status: Patch (to be ported) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.