Re: About libreoffice CVE
On 2017-11-24 10:14:20, Raphael Hertzog wrote:
> Hi,
>
> On Thu, 23 Nov 2017, Antoine Beaupré wrote:
>> > sal_uInt16 nLevelAnz;
>> > rIn >> nLevelAnz;
>> > if ( nLevelAnz > 5 )
>> > {
>> > OSL_FAIL( "PPTStyleSheet::Ppt-TextStylesheet hat mehr als 5 Ebenen! (SJ)" );
>> > nLevelAnz = 5;
>> > }
>>
>> I have taken on the Libreoffice DLA and I looked into this, but I didn't
>> notice that check. So I backported the patch anyways. It would have been
>> useful to mark CVE-2017-CVE-2017-12607 as N/A in CVE/list to avoid that duplicate
>> work... But I'm not sure your analysis is correct - the upstream patch
>> for that issue concerns an earlier part of the code:
>>
>> https://cgit.freedesktop.org/libreoffice/core/commit/?id=334dba623dfb0c4fb2b5292c2d03741b7b33aef1
>>
>> namely:
>>
>> - while ( rIn.GetError() == 0 && rIn.Tell() < aTxMasterStyleHd.GetRecEndFilePos() && nLev < nLevelAnz )
>> + while (rIn.GetError() == 0 && rIn.Tell() < aTxMasterStyleHd.GetRecEndFilePos() && nLev < nLevelAnz && nLev < nMaxPPTLevels)
>>
>> ... which sits about 100 lines above. Now I didn't check the upstream
>> code to see if it has that check we have in wheezy, but it seems it
>> won't hurt to add that patch anyways.
>
> It can't sit 100 lines above since it's using the variable that has been
> declared in the snipped that I pasted.
It sure can! The variable is declared twice in
PPTStyleSheet::PPTStyleSheet(), lines 4020-4026, where the patch
applies:
4040: sal_uInt16 nLevelAnz(0);
4041: rIn >> nLevelAnz;
4042:
4043: sal_uInt16 nLev = 0;
4044: sal_Bool bFirst = sal_True;
4045: bFoundTxMasterStyleAtom04 = sal_True;
4046: while (rIn.GetError() == 0 && rIn.Tell() < aTxMasterStyleHd.GetRecEndFilePos() && nLev < nLevelAnz && nLev < nMaxPPTLevels)
Then, later, there's your snippet, lines 4110-4116:
4110: sal_uInt16 nLevelAnz;
4111: rIn >> nLevelAnz;
4112: if ( nLevelAnz > 5 )
4113: {
4114: OSL_FAIL( "PPTStyleSheet::Ppt-TextStylesheet hat mehr als 5 Ebenen! (SJ)" );
4115: nLevelAnz = 5;
4116: }
Granted, it's not 100 lines, it's 60. :)
> The code I pasted is an old version
> of this current code:
>
> sal_uInt16 nLevelAnz(0);
> rIn.ReadUInt16(nLevelAnz);
>
> So I think that my analysis is correct.
My concern about the upstream code was that it may have mistakenly
removed the "if ( nLevelAnz > 5 )" check we have in wheezy, which would
have created the vulnerability.
I have just checked, and the check is still there:
https://sources.debian.net/src/libreoffice/1:5.4.0-1/filter/source/msfilter/svdfppt.cxx/#L4207
So I still believe the patch is relevant to wheezy.
[...]
>> So we should issue a regression update. We can probably do this
>> separately than a DLA for CVE-2017-12607 and CVE-2017-12608 though... In
>> fact, shouldn't we *always* issue separate DLAs for regression updates?
>
> I think it's fine to fix a regression together with other new security
> vulnerabilities.
I thought regressions had specific version numbers, related to the DLA
that introduced the vulnerability... Isn't there some feature there we
would miss by fixing a regression without relating the DLA number to the
DLA that introduced it?
On 2017-11-24 13:54:29, Raphael Hertzog wrote:
> On Thu, 23 Nov 2017, Antoine Beaupré wrote:
>> Now, I notice that the original advisory is about embeded data from the
>> network, so maybe I'm doing things wrong and I need a weirder use
>> case. In that case, I'd be happy to improve my test case to be able to
>> reproduce, but otherwise we're just shooting in the dark here.
>
> You might want to try to embed stuff from different files AIUI. You open
> a writer document and paste into it stuff coming from a spreadsheet
> (cells, graphics, etc.), and so on.
I tried that - no luck either.
I think I got a pretty good patchset now, attached. I'll start a build
from scratch and provide test packages.
A.
--
À mesure que l'opression s'étend à tous les secteurs de la vie,
la révolte prend l'allure d'une guerre sociale.
Les émeutes renaissent et annoncent la révolution à venir.
- Jean-François Brient, de la servitude moderne
Reply to: