Develop sql injection bug

[mgage]

This addresses the issues in bug 3792 - blind sql injection from Library browser commands.

All library browser related database queries were converted to parameterized queries.
references: http://search.cpan.org/~timb/DBI-1.636/DBI.pm#selectall_arrayref

The first check is to make sure that all of the pull down selections for finding problems of a certain type work. Check both the regular and advanced search selections.

Next is to try to construct an sql injection and see if it works. I don't have specs for doing this beyond what is in bug 3792 -- but I'm sure references can be found on the web.

[pstaabp]

Did you mean to merge this into develop instead?

[mgage]

Yes. I'm out running at the moment. I'll reassign it when I get back. You may be able to do it yourself. On Jun 24, 2017 10:31 AM, Peter Staab <[email protected]> wrote: Did you mean to merge this into develop instead? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_webwork2_pull_779-23issuecomment-2D310841961&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=o8_wMUHNKnYh6Iu_Ix2dOwKAxUPr8NG3sRf95NX3WYM&s=2UflLtyDPHz5esSolAgP87w5vTmINnXkrBbmOKnDOGE&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABJdfL-2Dv6389Q6A3xzXEQxJq-2Dz2G-2DNnLks5sHR3BgaJpZM4OEUvQ&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=o8_wMUHNKnYh6Iu_Ix2dOwKAxUPr8NG3sRf95NX3WYM&s=-VDUblBKOxOsGrRikgEMsN1EqKYLJe-NV2-7t7Cxd6E&e=>.

[Alex-Jordan]

Did you mean for this to go into develop?

[Alex-Jordan]

Oh, I see you are on it.

[Alex-Jordan]

I think I was able to reassign it.

[pstaabp]

@mgage did you think we should get this into 2.13 or wait until 2.14?

[mgage]

It’s and actual security hole so I think we should get it into 2.13. (The scoring bug was also and it was intended to be a hot fix merged in late January — but it slipped under the radar.) The fix is short and pretty straightforward (just use parameterized dbi queries instead of straight queries) and very localized (just the library browser queries) — the main thing is to make sure that it doesn’t break anything. I’m less concerned about testing to see whether it blocks every possible sql exploit. The fix will for sure be better than the current situation as long as no functionality breaks. I’ll put the current develop up on hosted2 this afternoon and invite people to make all kinds of queries in the library browser on that machine. Perhaps we can get some testers who would be willing to spend a little time but don’t have the resources to create their own development branch. I’d like to pull the sql injection, finish the testing on the Email replacement and then branch 2.13. Take care, Mike

…

Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_webwork2_pull_779-23issuecomment-2D311083175&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=sHAxx-byS5vOhEwBdmFNj5hXAoCdrsmL4ZpcpfUuu5w&s=fFWo49dFYslPey_BnAV-oQvcAfsJHy3kWNxlwa473DY&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABJdfDdP1rle2Aj3qTlrggLaHqOdADqNks5sH8XRgaJpZM4OEUvQ&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=sHAxx-byS5vOhEwBdmFNj5hXAoCdrsmL4ZpcpfUuu5w&s=U9Nmua6T-axp3KG26SL4EY9IswvdV5DMq6sXFyucJ4M&e=>.

[pstaabp]

I'll try to get to it today then. I just added this to the list of items for 2.13.

[mgage]

That sounds good. Once we get 2.13 branched off I’ll start focusing full time on localization issues and the instructor ww3 interface. For localization plans. When I start introducing the use of utf8 everywhere (i.e. I’ll merge the outstanding PR from Goeff on that subject) I’ll need lots of help testing that existing problems don’t break (something everyone can help with) but particularly with identifying whether we have made it possible to deal with other character sets — particularly those used in Hungarian, Chinese, etc. as well as Spanish, French, German and the Scandinavian languages. I plan to make it possible to write or rewrite WW questions in all of those languages. Along those lines I would like help in reading through the some of the more modern literature on localization. We started the maketext() project at least 3 years ago, probably more, and I believe the technologies are changing — in particular to cover javasScript code. The localization project proceeded fairly slowly because at the time there was not a lot of demand and we didn’t have someone to take the lead full time to nudge it along. The situation is changing now. We started storing our translating files with the organization Transifex https://www.transifex.com/ <https://www.transifex.com/> our hosted site is at https://www.transifex.com/webwork/webwork2/ <https://www.transifex.com/webwork/webwork2/>. You can sign up there if you want to help with creating translation dictionaries for your language. (I can add additional maintainers to this site if someone would like to take the lead in this project.). Transifex has been advertising about ways to do translation on the fly. I don’t know whether that would be relevant to the WW project, but it’s worth looking into. Here are some resources. I think these are at least worth skimming to see if we want to make any changes in our procedures as we do this next big push for localization. In particular should we consider Transifex Live? https://www.transifex.com/resources/ <https://www.transifex.com/resources/> https://www.transifex.com/how-it-works/ <https://www.transifex.com/how-it-works/> https://docs.transifex.com/getting-started/getting-started-as-a-manager <https://docs.transifex.com/getting-started/getting-started-as-a-manager> https://docs.transifex.com/getting-started/translators <https://docs.transifex.com/getting-started/translators> https://docs.transifex.com/live/introduction <https://docs.transifex.com/live/introduction> https://docs.transifex.com/api/introduction <https://docs.transifex.com/api/introduction> https://www.transifex.com/blog/2016/new-transifex/?utm_source=subscribers-gl&utm_medium=email&utm_content=new-transifex&utm_campaign=nl-2016-06-insights-gl&mkt_tok=eyJpIjoiWlRjME5XUmtaR1kzWm1VMCIsInQiOiJXN0FmWlJNOGMyM3VcL0ZiQUFXRXNCSUtBV05jdlFuT3lwcURJbUtoY01NZHpzdjNJMFlzVmJxNUZTMkZ2SEkzd2FrZXNoREN5ZHVTU0lRTzNSdm9CbUM5NzRLZEM4ODJkbGp5OXJDT1VoNmM9In0%3D <https://www.transifex.com/blog/2016/new-transifex/?utm_source=subscribers-gl&utm_medium=email&utm_content=new-transifex&utm_campaign=nl-2016-06-insights-gl&mkt_tok=eyJpIjoiWlRjME5XUmtaR1kzWm1VMCIsInQiOiJXN0FmWlJNOGMyM3VcL0ZiQUFXRXNCSUtBV05jdlFuT3lwcURJbUtoY01NZHpzdjNJMFlzVmJxNUZTMkZ2SEkzd2FrZXNoREN5ZHVTU0lRTzNSdm9CbUM5NzRLZEM4ODJkbGp5OXJDT1VoNmM9In0=> Take care, Mike

…

On Jun 26, 2017, at 11:18 AM, Peter Staab ***@***.***> wrote: I'll try to get to it today then. I just added this to the list of items for 2.13. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_webwork2_pull_779-23issuecomment-2D311091035&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=zLNkGWqK_hVEHijnzWAV6QgTMGKOIaA7liNAY25Kajc&s=u6Nnk8tz8dWRxZdHSUPAbObb7u-NX6Batsx-fLcL5Yc&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABJdfCeX8dkIxwb7NrM6z4TEw34yG2Jwks5sH8vegaJpZM4OEUvQ&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=zLNkGWqK_hVEHijnzWAV6QgTMGKOIaA7liNAY25Kajc&s=pByy1SPYNF4Uasd21c3FAuDWvi-3MrqY8jTarhMEVE8&e=>.

[pstaabp]

@mgage when I run develop with the suggestion from Bug 3792 I don't get any results, thinking that it doesn't match. Did you get results back when library_sections is set to

Motivational applications (estimation)" and substring(@@version,1,1)=5#

Also, if need be, I wrote a script to test:

use HTTP::Request::Common;
use LWP::UserAgent;
use JSON;

use Data::Dump qw/dd/;

my $url  = 'http://localhost/webwork2';

my $req = POST 'http://localhost/webwork2/instructorXMLHandler',
     Content_Type => 'form-data',
     Content      => [
      xml_command => 'searchLib',
      session_key => 'I06G1niSzNsnJdFVZwQ8PxzZKdWBMafO',
      user => 'peter',
      library_name => 'Library',
      courseID => 'test',
      command => 'countDBListings',
      library_subjects => 'Calculus - single variable',
      library_chapters => 'Limits and continuity',
      library_sections => 'Motivational applications (estimation)',
      #library_sections => 'Motivational applications (estimation)" and substring(@@version,1,1)=5#',
];

$ua = LWP::UserAgent->new;
my $res = $ua->request($req);

dd decode_json($res->content);

# if you want to see the complete response
#dd $res;

but to use, you'll need to change the session_key to a currently logged in key and change the user. I'm trying other ways of injecting as well, but so far have come up empty. Have you contacted the person who originally submitting the bug about any other insight?

Overall, the library browser seems to work as above and it seems like this is more secure.

[mgage]

This script seems very useful. I’d like to see it beefed up a little bit and included in webwork/webwork2/t as a unit test. I’d like to see a lot of unit tests included in that directory. Then we can hook up cpan’s TAP::Harness and Test::More to automatically run regression tests. Once that is in place it will be pretty safe to do the code clean up that you and Alex Jordan have been suggesting. Without the unit tests it’s always a bit worrisome whenever any code changes are made. I’m working on fixing sage.pl at the moment since JT needs it for the online author program tomorrow. I’ll reply in more depth later. Take care, Mike

…

On Jun 26, 2017, at 2:09 PM, Peter Staab ***@***.***> wrote: @mgage <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_mgage&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=zD_c26myN9dxyGqhbHaotrnG2tjkOrCxrwcMWIH4ynU&s=7gh4T_npwvsRWHG9PuWO75wDHOoiAOC87ef0r6Bp4qg&e=> when I run develop with the suggestion from Bug 3792 I don't get any results, thinking that it doesn't match. Did you get results back when library_sections is set to Motivational applications (estimation)" and substring(@@Version,1,1)=5# Also, if need be, I wrote a script to test: use HTTP::Request::Common; use LWP::UserAgent; use JSON; use Data::Dump qw/dd/; my $url = 'http://localhost/webwork2'; my $req = POST 'http://localhost/webwork2/instructorXMLHandler', Content_Type => 'form-data', Content => [ xml_command => 'searchLib', session_key => 'I06G1niSzNsnJdFVZwQ8PxzZKdWBMafO', user => 'peter', library_name => 'Library', courseID => 'test', command => 'countDBListings', library_subjects => 'Calculus - single variable', library_chapters => 'Limits and continuity', library_sections => 'Motivational applications (estimation)', #library_sections => 'Motivational applications (estimation)" and substring(@@Version,1,1)=5#', ]; $ua = LWP::UserAgent->new; my $res = $ua->request($req); dd decode_json($res->content); # if you want to see the complete response #dd $res; but to use, you'll need to change the session_key to a currently logged in key and change the user. I'm trying other ways of injecting as well, but so far have come up empty. Have you contacted the person who originally submitting the bug about any other insight? Overall, the library browser seems to work as above and it seems like this is more secure. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_webwork2_pull_779-23issuecomment-2D311137590&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=zD_c26myN9dxyGqhbHaotrnG2tjkOrCxrwcMWIH4ynU&s=X3Sdh6RZGb7qDVzcn2qA3VCqnfFuedzKAsi76pET7es&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABJdfHg-5Fl54vH1efUl7VtVI8StNBbusjks5sH-5FPWgaJpZM4OEUvQ&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=zD_c26myN9dxyGqhbHaotrnG2tjkOrCxrwcMWIH4ynU&s=pdDuwbUSfkDcbRvePCYAnIzD13fT4yQssp9lBiqX_WQ&e=>.

[pstaabp]

I have found one case that locks up mysql on develop that this fixes. Using the above case and setting

library_subjects => 'Trigonometry" or dbsj.name="Geometry'

will not return anything from the develop branch (I waited about one minute--I imagine it's going through the entire database), but immediately returns 0 under this branch.

I have tried more nefarious injections on both code without success (which is good).

[mgage]

I think I'd like to have this merged into develop. It doesn't break existing code and it stops at least some exploits. I suggest we include developing good unit tests (possibly cribbed from the ones that you are using for perl dancer) in the 2.14 project. If you agree Peter could you merge it?

[pstaabp]

I misread this message. Do you mean, I should go ahead and merge this branch? Or were you talking about the test script I wrote to pull into yours?

If you'd like this merged, can you pull in my test script first?

[mgage]

I was confused also. I've merged your PR adding the files to the t directory into this pull request.