|
From: Christian B. <pos...@cb...> - 2007-10-05 01:16:32
|
Hello,
Am Donnerstag, 4. Oktober 2007 schrieb David Goodwin:
> Christian Boltz wrote :
> > just going through the svn commits...
>
> I'm glad someone else looks at them :)
;-)
> > I consider it security-critical to include the password in the HTML
> > code (browser cache etc.). Luckily, this code seems to be buggy -
> > at least, it never included the password for me.
>
> Ah, it should say $tPassword. I'd never intentionally display the
> unencrypted password in the form.
This isn't really better - if someone uses unencrypted passwords, there=20
will be a cleartext password again.
The correct and secure solution is not to insert the password at all.
> <insert comment about postfix admin's horrible templating and
> sanitisation structure />
<insert comment about duplicated code />
;-)
> <Smarty + Prepared statements would make me very happy />
I don't know Smarty good enough to add a statement, but I'm afraid it=20
would add some overhead. There are other ways to make the templates=20
easier...
Prepared statements would be a good idea - we would get rid of SQL=20
injections easily.
> > Argh, it seems admin/edit-admin.php needs some fixes...
> >
> > I just fixed some password-related bugs in edit-admin.php:
> > - When entering a password in the first field and leaving the
> > second one empty, the password was changed anyways.
>
> Er.. I thought my logic was something along the lines of :
>
> 1) Get password from template - if it matches the encrypted password
> belonging to that admin assume it's not changed and do nothing.
>
> 2) If it doesn't match the encrypted password, see if password2 is
> available, and if they match, update the user's password to the new
> value (after pacrypt'ing it).
See above.
> > - It was also changed to an empty password if you left both fields
> > empty. This is a bad idea because you often modify some admin
> > settings without even knowing his password.
>
> There would normally have been the 1st password field with a value
> within it, so this shouldn't happen (unless someone wants it to have
> an empty password)
Again: see above ;-)
> I guess my web browser was a little too eager in caching/filling
> password boxes in for me.
Another reason to leave them empty by default ;-)
> > One bug is remaining in admin/edit-admin.php:
> > When editing an admin, it does not take the "active" status from
> > the database. This means editing an admin always disables it
> > (unless you correct the checkbox yourself).
> > Can you please fix this?
>
> Hmm.. I think this is a pgsql vs mysql-ism - in that it works for
> PgSQL. I'll try and review the code again today.
After your change, it works :-)
Regards,
Christian Boltz
=2D-=20
> |``All mail clients suck. This one just sucks less.'' -me, circa 1995
> Diese Aussage ist heute gueltiger denn je! ("me" ist Michael Elkins!).
Pah. Mutt kann ja nichtmal die einfachsten Scriptw=FCrmer interpretieren.
Geh mir da wech mit. [> David Haller und Ratti in fontlinge-devel]
|
