[TYPO3-core] Access denied for old security bug

[Helmut Hummel]

Hi!

Am 19.06.13 17:13, schrieb Christian Weiske:

>>> The fix: https://review.typo3.org/#/c/21501/
>>
>> Nice idea to do it this way, unfortunately, this would introduce a
>> severe security problem. Please read my comment in Gerrit.
>
> Could you please describe why you insist on verifying the OpenID URL
> before starting the OpenID login process?

I insist on security and clarity.

In regard of security, your approach[1] clearly wins ;)
It is not possible to log in with a different OpenID as specified in the 
backend user record. With Dmitrys approach[2] I can. It basically 
re-introduces the vulnerability we fixed before[3][4].


So why do I still dislike your patch?

1. It breaks a feature[5]
2. You claim your patch as bugfix, while OpenID auth with google works 
fine when using the https://profiles.google.com/<profileid> URL without 
any modification
3. Your patch introduces a different OpenID handling path 
(identifier_select) which coincidently works together with the 
implemented way (user claims id). These two paths should be clearly 
separated code wise
3a. There is no way I'm aware of to find out what ID to specify in the 
user record without debugging the OpenID response
3b. It stays unclear for users what to specify in the user record and 
what to specify in the login field
3c. If your statement "Another issue with google is that they return 
different OpenID identifiers for the same account for different OpenID 
consumer domains." is true (I could not verify it), such an approach 
needs use a truly unique identifier provided by the OP to identify the 
relation to a TYPO3 user
3d. Such unique identifier should be easily retrievable for end users or 
administrators and should be entered in a different field, not the 
claimed id field


So what I suggest is:

Implement identifier_select (for different providers) in a different 
extension or at least separated from other code in the extension.

Since there are even more cool things the OpenID protocol provides 
(Attribute exchange, check_immediate etc.) we could make the code of the 
openid extension modular so that extensions (or the core itself) can 
hook into to make all these things happen.

But mixing functionality does not seem right to me.

Hope that explains a bit my reasoning behind my voting of the current 
patches.

Kind regards,
Helmut



[1]https://review.typo3.org/21373
[2]https://review.typo3.org/21501
[3]http://typo3.org/teams/security/security-bulletins/typo3-core/typo3-sa-2010-001/
[4]https://git.typo3.org/Packages/TYPO3.CMS.git/commit/275af93acf617eee3b189b567289a58b70794c26
[5]https://git.typo3.org/Packages/TYPO3.CMS.git/commit/19ea7938331bc198848eef8740170f6318ab028c

-- 
Helmut Hummel
Release Manager TYPO3 6.0
TYPO3 Core Developer, TYPO3 Security Team Leader

TYPO3 .... inspiring people to share!
Get involved: typo3.org