search

urbanadventurer / WhatWeb

Next generation web scanner
https://www.morningstarsecurity.com/research/whatweb
GNU General Public License v2.0
5.57k stars 907 forks source link

Fix escape_for_sql method #358

Closed juananpe closed 3 years ago

juananpe commented 3 years ago

The previous version was quite broken: https://github.com/urbanadventurer/WhatWeb/blob/3e7d5eeedcf678b5624955122097330f43563360/lib/logging/sql.rb#L46-L53

Specifically, this line is not doing what we suppose it'll do:

"'" + s.tr("'", "\'") + "'"

Apparently, it seems that a string like escape'me would be translated to escape\'me but that's not the case:

2.7.2 :055 > "escape'me".tr("'","\'")
 => "escape'me"

You can find a good explanation of why here.

So the escape_for_sql method wasn't really escaping anything o_O

Finally, I think that this if branch is unneded: https://github.com/urbanadventurer/WhatWeb/blob/3e7d5eeedcf678b5624955122097330f43563360/lib/logging/sql.rb#L48-L50 In fact, nil.to_s surprisingly works (it would be a clear npe exception in Java!) and casts nil to an empty string "", making the whole if block superfluous. 

2.7.2 :046 > b=nil
 => nil
2.7.2 :047 > b.nil?
 => true
2.7.2 :049 > b.to_s
 => ""
2.7.2 :050 > b.to_s.nil?
 => false
urbanadventurer commented 3 years ago

Thanks @juananpe! I really like how detailed you were with this bug! There must not have been many people using the SQL logging for this issue to have gone unnoticed for so long!