Issue 2550933
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:47 | rouilj | set | status: open -> fixed resolution: fixed messages: + msg5929 |
2017-02-02 04:24:12 | rouilj | set | messages: + msg5928 |
2017-02-01 15:52:16 | antmail | set | nosy:
+ antmail messages: + msg5927 |
2017-02-01 00:54:54 | rouilj | set | status: new -> open messages: + msg5926 |
2017-02-01 00:09:40 | rouilj | set | messages: + msg5925 |
2017-01-31 23:57:26 | rouilj | create |