Opened 6 years ago

Closed 6 years ago

#1780 closed bug (fixed)

multiple sql escaping when writing to memberstrads

Reported by: lantti Owned by: lantti
Priority: major Milestone: unassigned
Component: BW Database Keywords: sql escape
Cc: crumbking, jsfan

Description

In the comments of ticket #1750 crumbking reported:

There is somesting not really working here: See: http://www.bewelcome.org/groups/211 I added a link: <a href="http://www.bewelcome.org/wiki/Halle">Halle Wiki</a>

This happens after saving the form a couple of times:

<a href=\\\\\\\"http://www.bewelcome.org\\\\/wiki/Halle\\\\\\\">Halle Wiki</a>

Quick look in revealed that this input (group description) gets escaped at least in group.entity.php/setDescription and later in words.lib.php/ReplaceInMTrad. The following comments in words.lib.php suggest that escaping issues have been worked on already in the past: " temporary hack to undo the damage done by escaping in other places" " todo: find all references to ReplaceInMTrad and fix them" " Change by jeanyves on AUgust 18 2009: \r\n are kept, but \' are replaced by '" " jy : I think we came here with an already escaped string." " judging from the exception logs this is NOT TRUE. Instead we now have a massive sql injection exploit vector"

There is also some kind of heuristics code in InsertInMTrad to find out if a string is already escaped and then escape the string or not.

Change History (14)

comment:1 Changed 6 years ago by lantti

  • Owner set to lantti
  • Status changed from new to accepted

comment:2 Changed 6 years ago by lantti

A quick fix seems to be using single quotes instead of double quotes. One of the escaping hacks in there forgets to check for single quotes so the result seems to come out ok for those :)

comment:3 Changed 6 years ago by planetcruiser

  • Cc jsfan added

is this a result of the last release? if yes, we should release a hot-fix for this as soon as possible. see http://trac.bewelcome.org/wiki/Hotfix

comment:4 Changed 6 years ago by lantti

As far as I see the result of the last release in relation to this is that htmlpurifier on the output page now refuses the links with extra backslashes. It clearly was broken already before, but didn't cause problems. Also now the problems manifest only when somebody uses double quotes in their links.

comment:5 Changed 6 years ago by planetcruiser

ok, so could someone hotfix this please? :)

comment:6 Changed 6 years ago by lantti

I did a minimum fix that fixes just this issue. I have a feeling there is eventually more wrong with the calls to i18n module. The fix is commit c3bac886ef39ed5c0331e4c6785ce22e1f500d8e

comment:7 Changed 6 years ago by crumbking

We really should test this very carefully... ;-) I will do so ...

comment:8 Changed 6 years ago by lantti

Could somebody merge this commit into the master branch for deployment? My git skills seem to fail me already.

comment:9 Changed 6 years ago by planetcruiser

merged into master and deployed live. please test and close ticket if done.

comment:10 follow-up: Changed 6 years ago by guaka

Great it's fixed and all but isn't it better to leave hotfixes for a really critical bug?

We don't even have unit tests so there can always be unforeseen side effects with fixes like this, so I think it would be better to just leave them for a release. Or?

(Well, still thanks for all the good work, bachelor's wives and maiden's children are well trained ;)

comment:11 in reply to: ↑ 10 Changed 6 years ago by planetcruiser

Replying to guaka:

Great it's fixed and all but isn't it better to leave hotfixes for a really critical bug?

We don't even have unit tests so there can always be unforeseen side effects with fixes like this, so I think it would be better to just leave them for a release. Or?

so far the loose policy for hotfixes is to deploy them if an issue is discovered on www that was missed when testing the last release on alpha. this is what was the case here, it was an amendment to the last release, namely ticket #1750.

an let's not get started on unit tests in rox. ;)

comment:12 Changed 6 years ago by crumbking

Okay tested here: http://www.bewelcome.org/groups/211

Link is working now. Not really thoughtful test thought but better than nothing ;-)

comment:13 Changed 6 years ago by planetcruiser

lantti, can we close this?

comment:14 Changed 6 years ago by lantti

  • Resolution set to fixed
  • Status changed from accepted to closed

Ok, if nobody found problems with it (I didn't either) then I close it.

Note: See TracTickets for help on using tickets.