Roundup Tracker - Issues

Issue 2550933

classification
Typing mismatched password on user display results in traceback.
Type: crash Severity: urgent
Components: Web interface Versions: devel
process
Status: fixed fixed
:
: rouilj : antmail, rouilj
Priority: high :

Created on 2017-01-31 23:57 by rouilj, last changed 2017-02-13 04:07 by rouilj.

Messages
msg5924 Author: [hidden] (rouilj) Date: 2017-01-31 23:57
If you go to  the edit for for a user e.g. tracker/user3 and type in two
different passwords for the password fields, you get a traceback:

   <type 'exceptions.AttributeError'>: 'str' object has no attribute
'dummystr'

   Debugging information follows

    While evaluating the standard:'context/history' expression on line 163 


In cgi/templating.py PasswordHTMLProperty::plain calls:

        if isinstance(self._value, hyperdb.Password):
            value = self._value.dummystr()
        else:
            value = self._('[hidden]')
        if escape:
            value = cgi.escape(value)

I think the intent is to hide the hashed password but display the
schema. I'll bet this worked until the latest changes to better
preserve form data when there is an error. The string value assigned
to the password field is now assigned to _value. I think it used to
initialize _value from the password object in the database.

The password object (via JournalPassword) has a dummystr that represents
the password using the scheme used to encrypt the password and then displays
a placeholder for the encrypted password.

I can fix this by changing the code above to:

       if self._value is None:
            return ''
        if isinstance(self._value, hyperdb.Password):
            value = self._value.dummystr()
        else:
            value = self._('[hidden]')
        if escape:
            value = cgi.escape(value)
        return value

so this hides the password if it's a string.

Arguably an alternate fix could be to access the db and pull the
password from
here and call the dummystr() on it, but this only triggers when people
are changing
the password and the password field is masked so why waste the time
doing that.

I am not sure when the self._value.dummystr() code would be triggered. I
guess if
somebody had a custom index page that displayed the password field it
could be shown.

For history (and in case this helps others understand what is happening
if I got it wrong)
full traceback of the failure:

Traceback (most recent call last):
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/client.py",
line 1227, in renderContext
    result = pt.render(self, None, None, **args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/engine_zopetal.py",
line 92, in render
    getEngine().getContext(c), output, tal=1, strictinsert=0)()
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 192, in __call__
    self.interpret(self.program)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 666, in do_useMacro
    self.interpret(macro)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 411, in do_optTag_tal
    self.do_optTag(stuff)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 396, in do_optTag
    return self.no_tag(start, program)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 391, in no_tag
    self.interpret(program)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 689, in do_defineSlot
    self.interpret(slot)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 632, in do_condition
    self.interpret(block)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 632, in do_condition
    self.interpret(block)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 411, in do_optTag_tal
    self.do_optTag(stuff)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 396, in do_optTag
    return self.no_tag(start, program)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 391, in no_tag
    self.interpret(program)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 632, in do_condition
    self.interpret(block)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 236, in interpret
    handlers[opcode](self, args)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/TAL/TALInterpreter.py",
line 564, in do_insertStructure_tal
    structure = self.engine.evaluateStructure(expr)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/PageTemplates/TALES.py",
line 225, in evaluate
    return expression(self)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/PageTemplates/Expressions.py",
line 193, in __call__
    return self._eval(econtext)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/PageTemplates/Expressions.py",
line 188, in _eval
    return render(ob, econtext.vars)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/PageTemplates/Expressions.py",
line 94, in render
    ob = ob()
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/templating.py",
line 893, in history
    current[prop_n] = prop.plain(escape=1)
  File
"/home/rouilj/develop/roundup.dev/roundup.sysadmin/roundup/cgi/templating.py",
line 1557, in plain
    value = self._value.dummystr()
AttributeError: 'str' object has no attribute 'dummystr'
msg5925 Author: [hidden] (rouilj) Date: 2017-02-01 00:09
One issue with my patch is that the journal entry for
the most recent password changes shows as hidden:

2017-02-01 00:04:47	admin	set	password: {PBKDF2}*encrypted* -> [hidden]
2017-02-01 00:03:58	admin	set	password: {PBKDF2}*encrypted* ->
{PBKDF2}*encrypted*
2017-02-01 00:03:49	admin	set	password: {PBKDF2}*encrypted* ->
{PBKDF2}*encrypted*

The other two journal entries originally had hidden on the rhs of ->
but once I make another password change the [hidden] turns into {PBK...
again. The else clause in my change is generating the [hidden] string.
But I am not sure why.
msg5926 Author: [hidden] (rouilj) Date: 2017-02-01 00:54
I have the now more pythonic:

        if self._value is None:
            return ''
        try:
            value = self._value.dummystr()
        except AttributeError:
            value = self._('[hidden]')
        if escape:
            value = cgi.escape(value)
        return value

If you get the password fields wrong, I get a journal entry that looks like:

2017-02-01 00:42:14	admin	set	password: {PBKDF2}*encrypted* -> [hidden]
2017-02-01 00:41:28	admin	set	address: demo@example.com -> demo@example1.com

If I change something else (leaving the password fields blank)

I get this in the history:

2017-02-01 00:43:47	admin	set	address: demo@example1.com -> demo@example.com
2017-02-01 00:42:14	admin	set	password: {PBKDF2}*encrypted* ->
{PBKDF2}*encrypted*
2017-02-01 00:41:28	admin	set	address: demo@example.com -> demo@example1.com

so it looks like the failed attempt to change the password at 42:14
was still recorded.

If I again do a broken password change this is displayed as the history:

2017-02-01 00:43:47	admin	set	address: demo@example1.com -> demo@example.com
2017-02-01 00:42:14	admin	set	password: {PBKDF2}*encrypted* -> [hidden]
2017-02-01 00:41:28	admin	set	address: demo@example.com -> demo@example1.com

If I now do a proper password change I see:

2017-02-01 00:46:07	admin	set	password: {PBKDF2}*encrypted* ->
{PBKDF2}*encrypted*
2017-02-01 00:43:47	admin	set	address: demo@example1.com -> demo@example.com
2017-02-01 00:42:14	admin	set	password: {PBKDF2}*encrypted* ->
{PBKDF2}*encrypted*
2017-02-01 00:41:28	admin	set	address: demo@example.com -> demo@example1.com

again not sure why the 42:14 failed password attempt is showing up in
the journal or why is changes back and forth to hidden. If I do another
failed password attempt I get:

2017-02-01 00:46:07	admin	set	password: {PBKDF2}*encrypted* -> [hidden]
2017-02-01 00:43:47	admin	set	address: demo@example1.com -> demo@example.com
2017-02-01 00:42:14	admin	set	password: {PBKDF2}*encrypted* ->
{PBKDF2}*encrypted*
2017-02-01 00:41:28	admin	set	address: demo@example.com -> demo@example1.com

despite the fact it's now 00:47 something. Very odd.

But at least displaying the password field in read only mode
(e.g. in user.index.html) works and displays values like:
{PBKDF2}*encrypted*

Unless somebody has a brighter idea, this fixes a crash bug and the
journal issue can wait for another day.
msg5927 Author: [hidden] (antmail) Date: 2017-02-01 15:52
Hello, John.

> If you go to  the edit for for a user e.g. tracker/user3 and type in two
> different passwords for the password fields, you get a traceback:

> In cgi/templating.py PasswordHTMLProperty::plain calls:

>         if isinstance(self._value, hyperdb.Password):
>             value = self._value.dummystr()
>         else:
>             value = self._('[hidden]')
>         if escape:
>             value = cgi.escape(value)

I   don't   have  this  issue. But I use Jinja2 and my roundup-tracker
source  is slightly different:

 def plain(self, escape=0):
         """ Render a "plain" representation of the property
       """
        if not self.is_view_ok():
             return self._('[hidden]')

       if self._value is None:
             return ''
       value = self._value.dummystr()
       if escape:
             value = cgi.escape(value)
         return value

May  be  it doesn't matter. I guess that there is a value substitution
in  password input control in TAL template. I don't understand TAL so
this is only a guess.
msg5928 Author: [hidden] (rouilj) Date: 2017-02-02 04:24
Hi Anthony:

In message <1585208555.20170201185211@inbox.ru>, Anthony writes:
[ john wrote ]
>> If you go to the edit for for a user e.g. tracker/user3 and type in two
>> different passwords for the password fields, you get a traceback:
>
>> In cgi/templating.py PasswordHTMLProperty::plain calls:
>>
>>         if isinstance(self._value, hyperdb.Password):
>>             value = self._value.dummystr()
>>         else:
>>             value = self._('[hidden]')
>>         if escape:
>>             value = cgi.escape(value)

Blech. I screwed up and pasted my fixed code twice. The code above is
supposed to be what you have below.

>I don't have this issue. But I use Jinja2 and my roundup-tracker
>source  is slightly different:
>
> def plain(self, escape=0):
>         """ Render a "plain" representation of the property
>       """
>        if not self.is_view_ok():
>             return self._('[hidden]')
>
>       if self._value is None:
>             return ''
>       value = self._value.dummystr()
>       if escape:
>             value = cgi.escape(value)
>         return value

This is the code that breaks if you have the current development head.

If you have a release roundup version (e.g. 1.5.1) the code works. I
think this problem only happens if you have the commit:

  changeset:   5166:232c74973a56
  user:        Ralf Schlatterbeck
  date:        Mon Aug 22 22:19:48 2016 +0200

which makes sure that a form keeps the edited values if it fails a
check when submitted. See: http://issues.roundup-tracker.org/issue1408570

I have to do a bisect to make sure that's the (much wanted) change in
functionality that caused the issue.

>May be it doesn't matter. I guess that there is a value substitution
>in  password input control in TAL template. I don't understand TAL so
>this is only a guess.

This substitution should happen in the jinja code as well. It's
possible the substitution is only needed for older trackers.

See: http://issues.roundup-tracker.org/issue2550688 for details.

Can you check a password change in the journal/history and
verify that you see:

   {scheme}*encrypted*

If it looks like:

   {scheme}the actual password hash

we are leaking the hashed password. I claim this code should prevent
the leak regardless of the templating language but...
msg5929 Author: [hidden] (rouilj) Date: 2017-02-13 04:07
Fixed by commit 5c8808f55d93 (aka dc657fbbc790).

Tried the original code and if it threw an attribute error
assigned the value [hidden] to it.
History
Date User Action Args
2017-02-13 04:07:47rouiljsetstatus: open -> fixed
resolution: fixed
messages: + msg5929
2017-02-02 04:24:12rouiljsetmessages: + msg5928
2017-02-01 15:52:16antmailsetnosy: + antmail
messages: + msg5927
2017-02-01 00:54:54rouiljsetstatus: new -> open
messages: + msg5926
2017-02-01 00:09:40rouiljsetmessages: + msg5925
2017-01-31 23:57:26rouiljcreate