Hi, I have just been looking over Picketlink and I think I have spotted a couple of thread safety issues: - File Data Store is not thread safe It looks like there are quite a few problems here, but the biggest is that FileDataStore does not seem to use any sync, so multiple threads can be attempting to write out the database at the same time. Also threads can be modifying the database in memory at the same time it is being written out, so it is possible to write the DB in an inconsistent state. Also when the file is written out it is written out directly over the old file, which greatly increases the chance of file corruption (rather than writing a tmp file and then moving it over the existing one). The also means that any sort of error (such as a non-serializable attribute) will corrupt the store and make it unreadable. - LDAPIdentityStore is using SimpleDateFormat in a non-threadsafe manner LDAPIdentityStore uses a static SimpleDateFormat, which is not thread safe. Not only that but this date format is modified before it is used in LDAPIdentityStore#parseLDAPDate, so if multiple threads are parsing dates with different timezone formats at the same time anything could happen. Stuart

Hi, I have just been looking over Picketlink and I think I have spotted a couple of thread safety issues: - File Data Store is not thread safe

It looks like there are quite a few problems here, but the biggest is that FileDataStore does not seem to use any sync, so multiple threads can be attempting to write out the database at the same time. Also threads can be modifying the database in memory at the same time it is being written out, so it is possible to write the DB in an inconsistent state. Also when the file is written out it is written out directly over the old file, which greatly increases the chance of file corruption (rather than writing a tmp file and then moving it over the existing one). The also means that any sort of error (such as a non-serializable attribute) will corrupt the store and make it unreadable.

- LDAPIdentityStore is using SimpleDateFormat in a non-threadsafe manner LDAPIdentityStore uses a static SimpleDateFormat, which is not thread safe. Not only that but this date format is modified before it is used in LDAPIdentityStore#parseLDAPDate, so if multiple threads are parsing dates with different timezone formats at the same time anything could happen.

Stuart

From: "Bill Burke" <bburke(a)redhat.com&gt; To: security-dev(a)lists.jboss.org Sent: Tuesday, 6 August, 2013 5:52:15 PM Subject: Re: [security-dev] Picketlink thread safety issues On 8/6/2013 11:01 AM, Anil Saldhana wrote: > On 08/06/2013 02:02 AM, Stuart Douglas wrote: >> Hi, >> >> I have just been looking over Picketlink and I think I have spotted a >> couple of thread safety issues: >> >> - File Data Store is not thread safe >> > Anything in particular? > >> It looks like there are quite a few problems here, but the biggest is that >> FileDataStore does not seem to use any sync, so multiple threads can be >> attempting to write out the database at the same time. Also threads can >> be modifying the database in memory at the same time it is being written >> out, so it is possible to write the DB in an inconsistent state. >> >> Also when the file is written out it is written out directly over the old >> file, which greatly increases the chance of file corruption (rather than >> writing a tmp file and then moving it over the existing one). The also >> means that any sort of error (such as a non-serializable attribute) will >> corrupt the store and make it unreadable. > We should look at how Wildfly does with serializing domain model changes > into the file. Maybe we need to retain copies so that the runtime can > recover from a corrupted file. >> - LDAPIdentityStore is using SimpleDateFormat in a non-threadsafe manner >> >> LDAPIdentityStore uses a static SimpleDateFormat, which is not thread >> safe. Not only that but this date format is modified before it is used in >> LDAPIdentityStore#parseLDAPDate, so if multiple threads are parsing dates >> with different timezone formats at the same time anything could happen. > We should move the SDF into the static method that is using it. You may also think about creating a transactional interface for the IDM API. For the JPA plugin it would just delegate to the EntityManager. For the File plugin, you might want to keep changes in memory (even keep them local to the IDM session) and flush them when the user calls "commit". This creates a lot more work though... Bill -- Bill Burke JBoss, a division of Red Hat http://bill.burkecentral.com _______________________________________________ security-dev mailing list security-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/security-dev

The file-based identity store was never intended to be used in a production environment, however it's just come to my attention that the file store will be the default configuration in PicketLink when embedded with WildFly. With this in mind (and the fact that the final release is coming up soon) we've decided to bump the issue to the 2.5.1 release to ensure that it gets the time required to implement properly. On a side note, Pedro has tested the FileIdentityStore with JMeter running 500 threads and there was no file corruption - I know this isn't definitive proof that there isn't a potential issue, however it should give us some confidence that it won't break under a moderate production load. On 07/08/13 06:36, Stuart Douglas wrote: > I did something similar for the EJB timer data store in Wildfly, however to simply it I stored each timer in its own file. The timers are written out after transaction commit, and written out to temp files before replacing the originals. > > In theory it is possible to write a single file based implementation that is much faster, but file per timer is much more durable than a non-transactional implementation, as it is much less likely that a problem will corrupt your entire store. Also unless you spend a lot of man hours writing some kind of concurrent transactional file based storage the performance is probably going to be about the same anyway. > > Stuart > > ----- Original Message ----- >> From: "Bill Burke" <bburke(a)redhat.com&gt; >> To: security-dev(a)lists.jboss.org >> Sent: Tuesday, 6 August, 2013 5:52:15 PM >> Subject: Re: [security-dev] Picketlink thread safety issues >> >> >> >> On 8/6/2013 11:01 AM, Anil Saldhana wrote: >>> On 08/06/2013 02:02 AM, Stuart Douglas wrote: >>>> Hi, >>>> >>>> I have just been looking over Picketlink and I think I have spotted a >>>> couple of thread safety issues: >>>> >>>> - File Data Store is not thread safe >>> Anything in particular? >>> >>>> It looks like there are quite a few problems here, but the biggest is that >>>> FileDataStore does not seem to use any sync, so multiple threads can be >>>> attempting to write out the database at the same time. Also threads can >>>> be modifying the database in memory at the same time it is being written >>>> out, so it is possible to write the DB in an inconsistent state. >>>> >>>> Also when the file is written out it is written out directly over the old >>>> file, which greatly increases the chance of file corruption (rather than >>>> writing a tmp file and then moving it over the existing one). The also >>>> means that any sort of error (such as a non-serializable attribute) will >>>> corrupt the store and make it unreadable. >>> We should look at how Wildfly does with serializing domain model changes >>> into the file. Maybe we need to retain copies so that the runtime can >>> recover from a corrupted file. >>>> - LDAPIdentityStore is using SimpleDateFormat in a non-threadsafe manner >>>> >>>> LDAPIdentityStore uses a static SimpleDateFormat, which is not thread >>>> safe. Not only that but this date format is modified before it is used in >>>> LDAPIdentityStore#parseLDAPDate, so if multiple threads are parsing dates >>>> with different timezone formats at the same time anything could happen. >>> We should move the SDF into the static method that is using it. >> >> You may also think about creating a transactional interface for the IDM >> API. For the JPA plugin it would just delegate to the EntityManager. >> For the File plugin, you might want to keep changes in memory (even keep >> them local to the IDM session) and flush them when the user calls >> "commit". This creates a lot more work though... >> >> Bill >> >> -- >> Bill Burke >> JBoss, a division of Red Hat >> http://bill.burkecentral.com >> _______________________________________________ >> security-dev mailing list >> security-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/security-dev > _______________________________________________ > security-dev mailing list > security-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/security-dev _______________________________________________ security-dev mailing list security-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/security-dev