BenBE's humble thoughts Thoughts the world doesn't need yet …


Debian Lenny and the Patches …

Filed under: GeSHi — Schlagwörter: , , , — BenBE @ 03:08:51

In May, June, July and August Milian and me were working on rewriting and optimizing some parts of the parser which caused a lot of performance gains, but also some trouble as I notice now. The problem is less that the code got broken somehow and fixed in some way again – which it sure got 😉 – but that it’s hard to reproduce how we did the fixing back then.

So what I’m refering to is a problem Secunia noted two releases after the initial patch for it. Back in GeSHi there existed a problem, that could cause GeSHi to go into an infinite loop – I already reported about this on the news page twice. In GeSHi 1.0.8 this issue was fixed and after the release an advisory asking for an update was issued. In work mainly concentrated on fixing the bugs in 1.0.8 thus no major work was done on the parser. The only big news in that version was a fix to a bug that had been in GeSHi since its early days and although I explicitely said there’s no real use for it in an attackers point of view Secunia started to report it which got the stone rolling.

Three days later – on the 3rd of November – I got mail from Romain Beauxis from the Debian Maintainance team caring about the php-geshi package asking me about details on the report filed by Secunia. I didn’t instantly recognize it as the bug I fixed in but when I finally did looked up the patch I had applied back then – which was quite easy as only a few lines changed and I exactly remembered where – and sent back my answer; including a remark they should concentrate on the bug more as that one was the more critical of both – even though Secunia didn’t even feel like mentioning it.

And that’s wher the work begins and the real trouble starts. Although I have an easy testcase there appeared a lot of questions; „What was the patch?“ being the most interesting while the least answerable of all. So the research began, only having some little hints like the approximate date of the fix, probable revisions falling in that time, the approximate place within the GeSHi source (which is about 500 lines that could be THE place!!!) and the beforementioned testcase

Following up on the mail discussion I had with the package maintainers at Debian some more details reentered the scene which shrinked the space of solutions some more. Within the changes there often have been minor changes where only few lines changed – including some changes where only formatting got changed to ease reading. All in all, about 150 revisions felt into the given range of dates, where still about 50 revisions did change geshi.php in some way – most of them affecting the faulty routine.

Besides the mentioned formatting changes there were structural and functional changes – like a new aspect of the parser getting implemented or the way some old task was performed getting optimized. If you simply skipped over the different patches there was one revision that sent out some strange „feeling“ when you read its commit comment- though you couldn’t really verify if it was the given one commit we were looking for.

So at first there had to be some look on the parts that could be affected. In parse_code there only were two loops that could easily get into hanging if you weren’t cautious enought. The first was the loop looking for the strict code blocks withing the source and the second being the loop for highlighting one such strict code block by stepping through it. And that’s where memories and luck popped in. Inspired by a mail from the Debian maintainers I where Nico Golde simplified the view on the first of the mentioned loops, I suddently remembered that Milian and me fixed some positioning code that was there to tell GeSHi where to continue after it found certain patterns. So I rechecked that portion of the code Nico had simplified in the version that was released and found the same oddity I just stumbled upon in the simplified code if Nico.

In the end I wrote a small test script and single-stepped it in the debugger … and there it was: The debugger looped exactly where I expected it to do when I was using the testcase. Given that all summed up and I verified the revision those lines were last changed: Exactly the revision I already felt strange about when I first had a broad view on that issue one week earlier.

Current state is that my findings are on their way to Nico Golde and Romain Beauxis and hopefully into an patched version soon!

To sum things up on the problem you could say the following: Even if there is a patch, and you know the issue, it’s not always as simple to bring both information together as sometimes the glue for both is missing as not every issue forcibly has an ID in the bugtracking system – sometimes intentionally and sometimes because you missed to do so. What was the case with this particular issue is hard to say especially when taking into account, that you aren’t always free on your commit comments as giving to much details there might help the bad guys to be faster while being to unspecific might let you loose the connection between the comment and the problem you solved.

Remains some last topic: Why didn’t the Debian guys simply update: After some mail exchange that question was answered: They simply couldn’t as there was a feature freeze in effect prohibiting them from including any patch that added or removed any feature from the package only leaving the way open to (important) patches that focussed on correcting security issues like both the Denial of Service problem in and the Remote Code Execution in 1.0.8 had been. And even though there had been attemps to get the newer release into Lenny – which would have saved a lot of work – none of them was successful making up just much more work than there would have been necessary in the first place.

If you do your work later it not only gets more and harder, it also will involve much more power of others than necessary if it was done in time.

P.S.: Sorry for not being specific on some topics – I promise to get back on the details as soon as the Debian people have fixed the problem in their package.

Flattr this!

Powered by WordPress