Re: [Phpgroupware-developers] Re: [ phpgroupware-Bugs-445721 ] email password not saved.

[Miles Lott]

I have just committed the ability to configure the mcrypt
algorithm and mode in setup.  To get this, you need to update
phpgwapi/inc/class.crypto.inc.php, admin/inc/hook_config.inc.php,
admin/templates/default/config.tpl, and setup/lang/phpgw_en.lang.

You also need the newer common and sessions classes committed
earlier today.

Of course, you also need to know what you are doing.  But, the
default is mentioned and also set, making behavior the same as
before unless you change it.

It detects if mcrypt is available, and lists the algos/modes,
checking for duplicates (some of mine were listing twice).

If not, it then sets the default anyway to tripledes/cbc.
This shouldn't hurt the non-mcrypt folks.

I was able to switch on the fly being logged into setup and
into phpGroupWare, and with debugging enabled in crypto class
I could see the working results.

Miles Lott wrote:
> 
> In the interest of public scrutiny, I am finding where some
> of the problems lie.
> 
> crypto is used (now) for sm, email, calendar, and appsessions.
> Now the version that email calls is apparently the direct call
> in crypto.  app_session and others have been calling a wrapper
> for encrypt/decrypt in the common class.  This wrapper also has
> been serializing/deserializing, for about a year now.
> 
> My recent problems pretty much started with my local upgrade
> to php 4.1.0 with mcrypt.  Since then i have often been getting
> '' back from appsessions via common and crypto for anything but
> a string.
> 
> What I am committing now is:
> 
> 1. Remove serialize/unserialize from common->(en/de)crypt.
>    That makes this wrapper transparent, but it is
>    probably better in practice to call a common class
>    function.  It is felt that a call to crypto directly
>    should not be done.
> 
> 2. serialize then addslashes in crypto->encrypt.  But, first
>    do a test to see if the data is an array or object.  Only
>    addslashes if using mcrypt.  In this way, strings are never
>    escaped.  This seems to work even with strings containing
>    single or double quotes, but not for double slashes of course.
> 
>    The whole point of doing addslashes here seems to be, as
>    I think Del pointed out, to overcome a problem with mcrypt
>    and the php serialized data.  Here, mcrypt returns '' if
>    I only serialize an array or object with quotes.
> 
>    Also, any db insert should probably be using db_addslashes.
>    It is not really crypto's job to make the data ready for
>    a db, only to it encrypt or decrypt it.
> 
> 3. stripslashes(mcrypt) then unserialize in crypto->decrypt.
>    Test to see that the result contains data.  At least in php4,
>    if you unserialize an invalid string, you get '' back.
>    As angles pointed out, this is actually boolean False.
>    If the data is still there, return it.  Otherwise, return
>    the 'unmolested' data.  I am not certain that we can
>    accurately gettype on the returned data here, so this is
>    the method I am using to determine if the data had been
>    serialized (array or object).
> 
> 4. In appsession (sessions class), call db->addslashes to
>    fix the data prior to insert to the db.
> 
> 5. Make the encryption algorithm (and mode) into class vars.
>    This would then be added to setup as a configuration option,
>    Which would be checked in the crypto class constructor.
> 
> I tested this here on php 4.1.0/4.0.5 with mcrypt and php
> 4.04pl1/3.0.18 without mcrypt and this seems to work. Skeeter
> and knecke have also tested it with no problems.
> 
> Miles Lott wrote:
> >
> > Please do not commit any more changes to the crypto or
> > sessions classes.  Submit patches to one of the project
> > admins.
> >
> > Tony (Angles) Puglisi wrote:
> > >
> > > Miles,
> > > There have been no "changes" to _existing_ crypto, we have a seperate,
> > > developmental set of en/de crypt functions that do not effect existing 
> > > apps, these
> > > functions are committed as such EXACTLTY for the purpose of PUBLIC review.
> > >
> > > In fact, these functions do NOTHING different yet, except correctly 
> > > "mirror image"
> > > the preparation of the thing to be encrypted.
> > >
> > > Crypto and Security needs the eyes of everyone to be effective. This 
> > > should be an
> > > ongoing effort with EVERYONE's input. Again, I commit on a seperate track 
> > > for
> > > public review and comment and involvement.
> > >
> > > Also, there has been NO change to any session items. Could you please 
> > > point out
> > > where that/these changes have occured?
> > >
> > > Miles Lott (address@hidden) wrote*:
> > > >
> > > >Please do not commit any more changes to the crypto or
> > > >sessions classes.  Submit patches to one of the project
> > > >admins.  Jengo most certainly needs to be involved in this.
> > > >
> > > >Del wrote:
> > > >>
> > > >> Tony (Angles) Puglisi wrote:
> > > >> >
> > > >> > Del,
> > > >> > Again, hack at it and sent me the file(s) (as attachments to an 
> > > >> > email). My
> > > brain is
> > > >> > too simple for these modern things, but I do know that I can commit 
> > > >> > a file :)
> > > >>
> > > >> OK, will do.  Not tonight, I've been driving for 10+ hours and my 
> > > >> brain is fried.
> > > >>
> > > >> One question before I start:  Do I have permission to change the 
> > > >> underlying
> > > encryption
> > > >> type for mcrypt 2.4 from 3DES (triple DES) to Rijndael?
> > > >>
> > > >> This will break pre-stored encrypted passwords, but provide an order 
> > > >> of magnitude
> > > >> performance and security benefit.  My understanding is that pre-stored 
> > > >> encrypted
> > > >> passwords are broken at the moment anyway.
> > > >>
> > > >> Any objections from the other developers?
> > > >>
> > > >> (trust me, I'm a cryptogeek:  DES should have gone into the dustbin of 
> > > >> history
> > > >> ages ago, and 3DES with it).
> > > >>
> > > >> > ahh.. as far as email goes, we only need to do crypto on a string, 
> > > >> > and a short
> > > >> > string at that, the pasword. Therefor to program for encrypting 
> > > >> > objects is over
> > > >> > kill for us but, hey, while we are at it why not do that...
> > > >>
> > > >> I agree, it's an easy operation to do it so we should do it.  It's no 
> > > >> major
> > > >> discomfort.  Note that you should always treat a string as an object 
> > > >> anyway
> > > >> (and therefore serialize it before encrypting) because of limitations 
> > > >> in the
> > > >> mcrypt functions.
> > > >>
> > > >> > As for database "de-fanging" (eliminitating database unfriendly 
> > > >> > chars and char
> > > >> > sequences), I believe this should be handled at the SO level, as an 
> > > >> > example,
> > > the
> > > >> > preferences class that handles the storage and retrieval of email 
> > > >> > prefs from
> > > the
> > > >> > preferences table, has code in there to handle adding slashes, and
> > > serialization.
> > > >> > (I don't know how other apps handle their prefs)
> > > >>
> > > >> OK, BUT:
> > > >>
> > > >> -  The mcrypt functions will ONLY handle an object / string that has 
> > > >> ALREADY been
> > > >>    de-fanged.  It will ALWAYS return a string (in fact a binhex'ed 
> > > >> string) that
> > > >>    has been de-fanged, or in any case does not need de-fanging.
> > > >>
> > > >> -  Since crypto will always return a de-fanged string when mcrypt is 
> > > >> enabled, why
> > > >>    not have it also always return a de-fanged string when mcrypt is 
> > > >> disabled?
> > > >>
> > > >> I agree that the encrypt & de-fang operations are logically 
> > > >> orthogonal, however
> > > in
> > > >> this case there is some benefit to having them combined due to the 
> > > >> reliance of
> > > one
> > > >> upon the other.
> > > >>
> > > >> --
> > > >> Del
> > > >>
> > > >> _______________________________________________
> > > >> Phpgroupware-developers mailing list
> > > >> address@hidden
> > > >> http://mail.gnu.org/mailman/listinfo/phpgroupware-developers
> > > >
> > > >--
> > > >
> > > >Miles Lott - http://milosch.net
> > > >phpGroupWare - http://www.phpgroupware.org
> > > >
> > > >_______________________________________________
> > > >Phpgroupware-developers mailing list
> > > >address@hidden
> > > >
> > > --
> > > that's "angle" as in geometry
> > >
> > > _______________________________________________
> > > Phpgroupware-developers mailing list
> > > address@hidden
> > > http://mail.gnu.org/mailman/listinfo/phpgroupware-developers
> >
> > --
> >
> > Miles Lott - http://milosch.net
> > phpGroupWare - http://www.phpgroupware.org
> >
> > _______________________________________________
> > Phpgroupware-developers mailing list
> > address@hidden
> > http://mail.gnu.org/mailman/listinfo/phpgroupware-developers
> 
> --
> 
> Miles Lott - http://milosch.net
> phpGroupWare - http://www.phpgroupware.org
> 
> _______________________________________________
> Phpgroupware-developers mailing list
> address@hidden
> http://mail.gnu.org/mailman/listinfo/phpgroupware-developers

-- 

Miles Lott - http://milosch.net
phpGroupWare - http://www.phpgroupware.org