Mantis Bugtracker          
testlink.org

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0004977TestLinkSecurity - Generalpublic2012-04-18 17:322015-11-07 11:28
Reporterfman 
Assigned Tokinow 
PrioritynormalSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version1.9.3 (2011 Q3 - bug fixing) 
Fixed in Version1.9.7 (2013 Q2 - bug fixing) 
Summary0004977: CSRF - Advisory ID: HTB23088 - Reference: https://www.htbridge.com/advisory/HTB23088 [^]
DescriptionAdvisory ID: HTB23088
Reference: https://www.htbridge.com/advisory/HTB23088 [^]
Product: Testlink
Vendor: teamst.org
Vulnerable Version(s): 1.9.3 and probably prior
Tested Version: 1.9.3
Public Disclosure: 9 May 2012
Vulnerability Type: Cross-Site Request Forgery (CSRF)
Risk Level: Low
Credit: High-Tech Bridge SA Security Research Lab (
https://www.htbridge.com/advisory/ [^] )



Advisory Details:

High-Tech Bridge SA Security Research Lab has discovered
vulnerabiliy in Testlink, which can be exploited to perform
?ross-Site Request Forgery (CSRF) attacks.


The application allows authorized users to perform certain
actions via HTTP requests without making proper validity
checks to verify the source of the requests. This can be
exploited to add, delete or modify sensitive information,
for example to change administrator\'s email.

An attacker should make logged-in administrator open a
malicious link in the browser to exploit this vulnerability.

The following PoC will change administrator\'s email:


<form action=\"http://[host]/lib/usermanagement/userInfo.php\" [^]
method=\"post\" name=\"main\" id=\"main\">
<input type=\"hidden\" name=\"doAction\" value=\"editUser\">
<input type=\"hidden\" name=\"lastName\" value=\"LastName\">
<input type=\"hidden\" name=\"firstName\" value=\"FirstName\">
<input type=\"hidden\" name=\"emailAddress\" value=\"new@mail.com\">
<input type=\"submit\" id=\"btn\">
</form>
<script>
document.getElementById(\'btn\').click();
</script>

TagsNo tags attached.
Database (MySQL,Postgres,etc)N/A
Browser
PHP Version
TestCaseID
QA Team - Task Workflow StatusREADY FOR TESTING
Attached Files? file icon TestLink-CSRF.odt [^] (23,392 bytes) 2012-05-28 20:53

- Relationships
parent of 0005317closedfman must login twice and logout before login. 
related to 0005148closedfman http://itsecuritysolutions.org/2012-08-13-TestLink-1.9.3-multiple-vulnerabilities/ [^

-  Notes
(0016715)
fman (administrator)
2012-05-20 07:45

Reminder sent to: kinow

do you can work on this ?
before comminting anything, please talk to me
we have done some HEAVY work on 2.0 regarding multiple tabs, and removing session usage.
DO NOT PLAN TO REDO on 1.9.4
(0016716)
kinow (reporter)
2012-05-20 23:25

Sure thing, I can work on this. I will read about it, prepare a way to reproduce this issue, and will check how to patch the code.

Then will post here my findings, so we can decide what to do next :-) No commits until then.

I have never worked with CSRF, it will be fun. Thanks!
(0016769)
kinow (reporter)
2012-05-25 18:42

Hi @fman

I've read about it, and even update an application (written in PHP+CodeIgniter) to use a CSRF protection. The best way to fix it that I could think of, is by adding a token to each form.

This token would have to be checked every time a form was posted. It would have a short expiration time. But even then, TestLink wouldn't be 100% CSRF-safe. A user could submit a form, and if the attacker could get this token, he could then proceed to use this token in his requests.

Using the referrer or http headers wouldn't work if the user browser didn't include the referrer header, for instance. And an attacker could change these headers. Same with Javascript, a browser without Javascript wouldn't work with TestLink.

So I think using a token in all forms with post is the best choice. We will have to make sure no sensitive information is changed through GET's. Otherwise, TestLink could still be attacked by a fake image URL sent in a HTML e-mail message. And for very risky operations, we could ask the user his password again (as is done in FaceBook/Twitter, sometimes).

Part of this task would be done easily in the index.php. We could check/generate/re-generate the CSRF token in the init_args() method.

But we would have to change a lot of Smarty tpl files. Basically, all the template files with forms, and include a hidden field with this token. Maybe we could create a Smarty helper code, so that we could use {form action='' ...} and it would create the form and include the CSRF token.

What do you think? Hope I've made my findings clear enough :-)

Have a great weekend.

Bruno
(0016770)
fman (administrator)
2012-05-25 19:58
edited on: 2012-05-25 20:10

Bruno:
we can not afford this kind of work for 1.9.4 => we are going to release without any fix.
We will fix this with more time on 2.0

I would like you to give a look to mantis code to understand what they have done, normally they had good solutions.
I've added time ago the method they use (cookie_string field on user table), we need to investigate if this measure can solve Cross-site request forgery

regards

http://www.mantisbt.org/bugs/view.php?id=8975 [^]
http://www.securiteam.com/unixfocus/5WP0N0AOAW.html [^]
https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29_Prevention_Cheat_Sheet [^]
https://www.owasp.org/index.php/PHP_CSRF_Guard [^]
http://simonwillison.net/static/2008/csrf_protect.php.txt [^]

(0016771)
fman (administrator)
2012-05-25 20:32

may be we can release 1.9.4 and then 1.9.5 with CSRF code
what do you think ?
(0016772)
fman (administrator)
2012-05-25 20:37

found this:
...After re-reading the OWASP documentation (Synchronizer Token Pattern section), it seems that one token per session is okay.
When I was originally looking into it a few years back, I could have swore OWASP was recommending a unique token per form/request (which is why I was regenerating a new one each time). They mention this in the last paragraph, but even say it has usability issues (which I am running into now with browser tabs).

we need to understand if (cookie_string field on user table) make this magic

have you done your test on 1.9.4 or 1.9.3 ?
(0016773)
fman (administrator)
2012-05-25 20:54

More:
CSRF

In most cases, you'll generate a unique token for each request and store it in the user's session. You would then submit this token value with a form submission (e.g. as a hidden field value); the submitted token would be compared to the existing session token when the request is received by your application. If the tokens match, you can assume the request is from your own application. If the tokens do not match, the request is likely from a third-party domain and should be ignored.
(0016774)
fman (administrator)
2012-05-25 20:59

Bruno I would like you produce a document with detailed steps, to allow me to reproduce the attack.
Please do it really for dummies.
do you think it will be possible ?
I would like to test myself 1.9.4
(0016775)
kinow (reporter)
2012-05-26 03:05

Hi Francisco!

> I would like you to give a look to mantis code to understand what they have done, normally they had good solutions.

Thanks for the links! I will have a look at Mantis code. The more examples we use the merrier. And will take a look at the links too.

> may be we can release 1.9.4 and then 1.9.5 with CSRF code
> what do you think ?

I think we can do it. We just need to have time to test it very carefully.

> have you done your test on 1.9.4 or 1.9.3 ?

Not yet, I will reproduce this issue, and will produce a test scenario for us, something we can use as simplest test.

> ...After re-reading the OWASP documentation (Synchronizer Token Pattern section), it seems that one token per session is okay.

In CodeIgniter you can configure to generate one token per request too. But the default is one per session, with a expiration time. Somewhere in the code, I read about a problem with more than one form issues with this approach.

> They mention this in the last paragraph, but even say it has usability issues (which I am running into now with browser tabs).

Maybe it's the same issue they had with multiple forms, or just similar.

> Bruno I would like you produce a document with detailed steps, to allow me to reproduce the attack.
> Please do it really for dummies.
> do you think it will be possible ?
> I would like to test myself 1.9.4

Cool, I will write a simple document, very basic with detailed instructions to reproduce the atack. I will try to produce this document this weekend, ASAP.

And I will try running this attack in TestLink 1.9.3 and 1.9.4.

Let me know if you have any more ideas, and thanks for the feedback! :D

-Bruno
(0016776)
fman (administrator)
2012-05-26 07:01
edited on: 2012-05-26 07:01

Is important to test on 1.9.4, not on 1.9.3 (we know 1.9.3 has the issue)

Thanks a lot for your help
(this week end I will work no much time on TL I've to go to a wedding party!!)

(0016777)
fman (administrator)
2012-05-26 07:38

Have found:
http://badboysofquality.wordpress.com/page/2/ [^]

that talk about several other sec issue on 1.8.5 => will be better if we can also fix this on 1.9.4/5
If needed open other issues please.
After reading do not think tha cookie_string already implemented can solve the issue.
My advice again is porting mantis code (core/form_api.php) to testlink
(reinventing the wheel,do not have IMHO any value for TL)
(0016778)
fman (administrator)
2012-05-26 07:41
edited on: 2012-05-26 07:47

More to check
(think I've solved some of these http://mantis.testlink.org/view.php?id=4906 [^])

79452 2012-02-20 TestLink lib/cfields/cfieldsEdit.php cfield_id Parameter SQL Injection
79453 2012-02-20 TestLink lib/plan/planMilestonesEdit.php Multiple Parameter SQL Injection
79454 2012-02-20 TestLink lib/requirements/reqImport.php req_spec_id
Parameter SQL Injection

http://packetstormsecurity.org/files/109983/TestLink-1.9.3-SQL-Injection.html [^]

(0016779)
fman (administrator)
2012-05-26 17:44

Bruno
would you mind to explain in detail:
>> Part of this task would be done easily in the index.php.
(0016780)
kinow (reporter)
2012-05-26 17:55
edited on: 2012-05-26 18:00

Sure, I *think* we could include the token generation and the verification there.

I thought all the requests pass by index.php, as I've seen the init_args function there, and in some other php files we use the $args variables, I assumed it was created in the index.php file.

Could you confirm this?

(0016781)
fman (administrator)
2012-05-26 18:27

Request are not routed through index.php.
each menu item launch a different php file
Work has to be done (as happens on mantis) on each page
(0016782)
kinow (reporter)
2012-05-26 18:37

You're right indeed, so we have a lot of work to do both in the template pages and in each php file.

Tonight I will start reading Mantis code, and will start to write the document for us, with a simple example of how to reproduce the attack.
(0016783)
fman (administrator)
2012-05-26 18:46

Ok.
Work can seem more daunting, that what really will be.
we are just going to have patiente.
I've got secureimage (a catcha for PHP), with this we can fix issue regarding changes user attributes, but we need to solve in a general way (as done by mantis) CSRF.

On 2.0 we need to work to improve security

regards
(0016784)
kinow (reporter)
2012-05-26 18:53

+1 on secureimage, I'm using secureimage in my websites too, I was using a captcha that was impossible to use (CodeIgniter default one), and reCaptcha was too complicated to set up. With secureimage I limited the characters and no more complaints about the captcha quality :-)
(0016785)
fman (administrator)
2012-05-26 19:02

ok
do you think has any sense add secureimage on TestLink?
on what feature ?

regards
(0016786)
kinow (reporter)
2012-05-26 19:45

Off the top of my head:

The registration screen. When you go to the login screen and click "New user?". A bot could fill that form I think.

The lost password screen too. So a bot also doesn't floods the user mail inbox with "Lost password" messages, sent by the system.

Maybe if the same person, tried to login multiple times, say, 10 times, we could show the captcha there too.

What do you think?

Cheers
(0016787)
fman (administrator)
2012-05-27 07:23

seems the usual use, ok.
Open new issue please, and set target 2.0
(0016793)
kinow (reporter)
2012-05-28 20:53

Hi @fman

Let me see if I got it right, we are going to create new issue for the captcha, for the cases above, right? If so I'll create the issue tonight, and I can start working on this if you need any help :-)

Attached you'll find a document describing how to reproduce the attack. And here's the project with examples of how to use the CSRF attack: https://github.com/kinow/reproduce-csrf-tl [^]

I will increment this document with a proposal on how to fix it :-)

What do you think?
(0016794)
fman (administrator)
2012-05-29 05:27
edited on: 2012-05-29 05:34

1. create new issue for captcha, but do not work on this.
2. analyse mantis form_api code in order to undertand how to use it on TL to generare a patch after 1.9.4
3. is better if you can get 1.9.4 and start testing it on Postgres focused on reports

let me know

(0016810)
kinow (reporter)
2012-05-31 17:32

Issue created @fman: http://mantis.testlink.org/view.php?id=5038 [^]

I've cloned the Mantis code, will read it tomorrow morning :-)

> 3. is better if you can get 1.9.4 and start testing it on Postgres focused on reports

Will do this, and in parallel will write some code to propose a fix for the CSRF code. Just to hang onto this momentum :-)

Cheers
(0016859)
kinow (reporter)
2012-06-11 23:06

Hi @fman,

I've successfully fixed the CSRF using the code from Mantis. Here's what I did to fix the issue when creating test suites (I used this example as we have some code to reproduce this issue [1]):

- Copied form_api.php from Mantis project to lib/functions
- Copied gpc_api (used to generate security code) from Mantis project to lib/functions
- Changed form_api.php removing call to some internal functions in Mantis
- Updated containerEdit.php with a call to a function from form_api.php that generates the security token
- Updated containerNew.tpl with a verification function from form_api.php

What should I do now? Do you want me to create a branch at my fork of testlink-code/master with this example? I believe the licenses from TestLink and Mantis are compatible, but should we literally copy the code, or adjust to TestLink (it will take a few days to polish the code)?

Thank you in advance! :)

[1] https://github.com/kinow/reproduce-csrf-tl/blob/master/create_suite.php [^]
(0016865)
fman (administrator)
2012-06-12 18:44

Great Job!.
Next step:
1. we need to provide instructions to users to apply changes to 1.9.3
2. would like to have a document for developers ( and for me) that explain step by step how pages have to be refactored.

3. You can start applying this changes on 1.9.4 (we will work on 2.0 in future),
(I would like to release 1.9.4 stable on August, with an beta on 15/7/2012).

4. regarding licensess, I have always taken code from mantis and adapt to our needs, mention on files that we have taken code from Mantis.
IMHO we can use code as is, but if you think thar' adjust to TestLink ' is a better option do your choice.
(0016870)
kinow (reporter)
2012-06-13 04:22

Yay Francisco!

I will have to adapt only a minor part of the code, just remove some calls to functions that are specific to mantis (although I believe there is more interesting code in mantis that we could use in TL :-)

I think it will take just one or two days to finish writing this doc with the patch instructions.

Will attach it here as soon as I've finished it.

Thanks! -B

Once I've done some progress, I will attach the document here
(0016921)
kinow (reporter)
2012-06-21 21:06

@fman: What do you think? -> https://gitorious.org/testlink-ga/testlink-code/commit/2d4ac941314f8bda80e265c9de8bacf17d1cd3e6 [^]

I will add the strings added in this commit to the locale/en_GB later, but first would like to hear if this is good enough to move on patching other forms :-)

Thanks!
(0016942)
kinow (reporter)
2012-06-30 03:11

So far applied the fix for:

- Login form - r1618ffb
- User information form (personal data, password and devkey) - r00f54c6
- Add test suite - r2d4ac94

Tomorrow I will look at a few more forms to apply the fix.
(0017035)
kinow (reporter)
2012-07-31 22:37

CSRF token included in test project forms: https://gitorious.org/testlink-ga/testlink-code/commit/b44337f00191b5b21d840b7b82ee792232932459 [^]
(0017036)
kinow (reporter)
2012-07-31 22:45

CSRF token included in test plan forms: https://gitorious.org/testlink-ga/testlink-code/commit/9d5726433be6f7f71c8d64942d6209f388f2693d [^]
(0017037)
kinow (reporter)
2012-07-31 23:59

CSRF token included in build forms: https://gitorious.org/testlink-ga/testlink-code/commit/aff1453794aff7b82923ece9a5751e9f1ced32f1 [^]
(0017836)
kinow (reporter)
2012-11-17 16:35

These two commits revert the old approach and apply another one proposed by OWASP (thanks @fman for the tip):

https://gitorious.org/testlink-ga/testlink-code/commit/eca44be8376148ee417481df16a556291dc0559b [^]
https://gitorious.org/testlink-ga/testlink-code/commit/346c737d56639bb59c2955fc40418e24627b7572 [^]

I believe if this passes the tests, we can close this issue and send the feedback to the HTBridge guys.
(0019486)
fman (administrator)
2013-09-04 04:53

Reminder sent to: kinow

hi can we set as closed ? there is people asking for confirmation
(0019488)
kinow (reporter)
2013-09-04 18:59

I think we can, it has been successfully used in 1.9.7 and I haven't heard any complains about it. Thanks again for that suggestion to use that OWASP approach instead of our old one. Way easier.
(0019489)
fman (administrator)
2013-09-04 19:46

thanks again for your help
(0024215)
fman (administrator)
2015-11-07 11:28

Github references
https://github.com/TestLinkOpenSourceTRMS/testlink-code/commit/eca44be8376148ee417481df16a556291dc0559b [^]

- Issue History
Date Modified Username Field Change
2012-04-18 17:32 fman New Issue
2012-04-18 17:32 fman Status new => assigned
2012-04-18 17:32 fman Assigned To => fman
2012-05-20 07:45 fman Note Added: 0016715
2012-05-20 23:25 kinow Note Added: 0016716
2012-05-25 18:42 kinow Note Added: 0016769
2012-05-25 19:58 fman Note Added: 0016770
2012-05-25 20:00 fman Note Edited: 0016770 View Revisions
2012-05-25 20:02 fman Note Edited: 0016770 View Revisions
2012-05-25 20:05 fman Note Edited: 0016770 View Revisions
2012-05-25 20:06 fman Note Edited: 0016770 View Revisions
2012-05-25 20:10 fman Note Edited: 0016770 View Revisions
2012-05-25 20:32 fman Note Added: 0016771
2012-05-25 20:37 fman Note Added: 0016772
2012-05-25 20:54 fman Note Added: 0016773
2012-05-25 20:59 fman Note Added: 0016774
2012-05-26 03:05 kinow Note Added: 0016775
2012-05-26 07:01 fman Note Added: 0016776
2012-05-26 07:01 fman Note Edited: 0016776 View Revisions
2012-05-26 07:38 fman Note Added: 0016777
2012-05-26 07:41 fman Note Added: 0016778
2012-05-26 07:47 fman Note Edited: 0016778 View Revisions
2012-05-26 17:44 fman Note Added: 0016779
2012-05-26 17:55 kinow Note Added: 0016780
2012-05-26 18:00 kinow Note Edited: 0016780 View Revisions
2012-05-26 18:27 fman Note Added: 0016781
2012-05-26 18:37 kinow Note Added: 0016782
2012-05-26 18:46 fman Note Added: 0016783
2012-05-26 18:53 kinow Note Added: 0016784
2012-05-26 19:02 fman Note Added: 0016785
2012-05-26 19:45 kinow Note Added: 0016786
2012-05-27 07:23 fman Note Added: 0016787
2012-05-28 20:53 kinow Note Added: 0016793
2012-05-28 20:53 kinow File Added: TestLink-CSRF.odt
2012-05-29 05:27 fman Note Added: 0016794
2012-05-29 05:34 fman Note Edited: 0016794 View Revisions
2012-05-31 17:32 kinow Note Added: 0016810
2012-06-11 23:06 kinow Note Added: 0016859
2012-06-12 18:44 fman Note Added: 0016865
2012-06-12 20:27 fman Assigned To fman => kinow
2012-06-13 04:22 kinow Note Added: 0016870
2012-06-13 04:22 kinow Status assigned => work in progress
2012-06-21 21:06 kinow Note Added: 0016921
2012-06-30 03:11 kinow Note Added: 0016942
2012-07-03 18:14 fman Summary Advisory ID: HTB23088 - Reference: https://www.htbridge.com/advisory/HTB23088 [^] => CSRF - Advisory ID: HTB23088 - Reference: https://www.htbridge.com/advisory/HTB23088 [^]
2012-07-03 18:14 fman Description Updated View Revisions
2012-07-31 22:37 kinow Note Added: 0017035
2012-07-31 22:45 kinow Note Added: 0017036
2012-07-31 23:59 kinow Note Added: 0017037
2012-08-17 08:54 fman Relationship added related to 0005148
2012-11-01 22:17 kinow Relationship added parent of 0005317
2012-11-17 16:35 kinow Note Added: 0017836
2013-09-04 04:53 fman Note Added: 0019486
2013-09-04 18:59 kinow Note Added: 0019488
2013-09-04 19:46 fman Note Added: 0019489
2013-09-04 19:46 fman Status work in progress => feedback
2013-09-04 19:46 fman QA Team - Task Workflow Status => READY FOR TESTING
2013-09-04 19:46 fman Status feedback => closed
2013-09-04 19:46 fman Resolution open => fixed
2013-09-04 19:46 fman Fixed in Version => 1.9.7 (2013 Q2 - bug fixing)
2015-09-15 21:09 fman Category Security => Security - XSS
2015-09-15 21:10 fman Category Security - XSS => Security - General
2015-11-07 11:28 fman Note Added: 0024215



Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker