search

cm12-amami / discussion

1 stars 0 forks source link

ASB 2018.09 #39

Closed MSe1969 closed 6 years ago

MSe1969 commented 6 years ago

September ASB is out - yet w/o AOSP links . . .

Observations: a. MM is also not updated anymore b. Pixel bulletin only contains kernel references

MSe1969 commented 6 years ago

Android Bulletin, 2018-09-01

Android Runtime

Framework

Library

Media Framework

System

Update Media Framework

Android Bulletin, 2018-09-05

Framework

Kernel components

Qualcomm components

Pixel/Nexus Bulletin

Kernel components

Qualcomm Components

MSe1969 commented 6 years ago

Backporting CVE-2018-9483 drives me nuts! C-Casting :( @hahnjo - I think you are more familiar with C, I'll upload the patch (i.e. my unsuccessful attempt of a backport) to the discussion repo, maybe you have a better idea - BD_ADDR is an array-typedef, which is passed to the function, and I am struggling whether now the array is passed or maybe only the pointer to its 1st element... (I don't really like C, by the way...)

MSe1969 commented 6 years ago

0001-BACKPORT-Don-t-use-Address-after-it-was-deleted.patch is the file name

MSe1969 commented 6 years ago

Update libxml2 to 2.9.8 I have set CVE-2018-9466 / CVE-2018-9472, which both point to the update of libxml2 to 2.9.8, to 'deferred' and added to our open CVE issue. We can't build it due to too many dependencies in the LP build tree; also usage as prebuilt (build in cm-14.1 and use compiled shared and static libs as prebuilts) won't work. So the only way, I see, is to maybe find and pick/backport the relevant single commits in the Github mirror https://github.com/GNOME/libxml2 ...? However, if Google has decided that it makes more sense to simply upgrade the lib, I doubt this will make a lot of sense...

Micha-Btz commented 6 years ago

I'm on vacation the next two weeks, so I can only help after this.

hahnjo commented 6 years ago

@MSe1969 can you post the error message that is failing the build?

The patch looks mostly good, having an array parameter in a function signature means "pass the pointer" IIRC. So maybe it's enough to say BTM_DeleteStoredLinkKey (bda, NULL); (without ampersand)?

MSe1969 commented 6 years ago

CVE-2018-9466, CVE-2018-9467, CVE-2018-9472 - Comments Those three commits are described in the ASB as "The most severe vulnerability in this section could enable a remote attacker using a specially crafted file to execute arbitrary code within the context of an application that uses the library."

CVE-2018-9467 adds the characters '\', '#' and '?', next to '/' as terminators when parsing the host part of a URL. In our LP coding, '#' and '?' are already considered, only missing was '\', so I performed the backport accordingly - please have a look and confirm, that the backport indeed is that simple

CVE-2018-9466/CVE-2018-9472 point in the ASB to the upgrade of libxml2 to 2.9.8, which - as already outlined - is not possible, because our version is too old and there are dependencies in the build tree. So as the "next best" approach, I have taken a couple of commits from https://github.com/GNOME/libxml2 , which address parsing issues potentially leading to endless loops, or other uncaught parsing errors. please advise, whether you consider this as an appropriate approach

MSe1969 commented 6 years ago

Hi @hahnjo - with regards to CVE-2018-9483, it is important to look at the context. Although I am pretty sure, I am not telling you anything you aren't already aware of, I would like to outline the broader context:

Our code in external/bluetooth/bluedroid is written in C, whilst the N/O/P code in system/bt is C++ The code was simply taken and, besides formatting, only adapted by Google, where C++ differs from C. And afterwards of course further fixes applied...

In the coding of CVE-2018-9483, the function parameters 'remote_bd_addr' and 'bd_addr' are in fact copied into new variables and those copies are passed on to the next functions. In O/P those are of type 'RawAddress', whilst in our LP code, the same parameters are passed as type 'BD_ADDR'. In LP, BD_ADDR is a typedef of an UINT8 length 6 array, in O/P, RawAddress is an own class (see types/raw_address.h).

My first attempt was to simply replace the declaration of the 'copy variables', simply to say e.g. BD_ADDR addr_copy = remote_bd_addr; instead of RawAddress addr_copy = remote_bd_addr; and BD_ADDR bda = p_dev_rec->bd_addr; instead of RawAddress bda = p_dev_rec->bd_addr;.

Result:

external/bluetooth/bluedroid/bta/./dm/bta_dm_act.c:4137:9: error: invalid initializer
         BD_ADDR addr_copy = remote_bd_addr;
         ^

Splitting declaration and assignment BD_ADDR addr_copy; addr_copy = remote_bd_addr; leads to 

external/bluetooth/bluedroid/bta/./dm/bta_dm_act.c:4137:38: error: incompatible types when assigning to type 'BD_ADDR' from type 'UINT8 *'
         BD_ADDR addr_copy; addr_copy = remote_bd_addr;
                                      ^

And finally using 'memcpy' for array-processing (my uploaded patch file) brings

external/bluetooth/bluedroid/stack/./btm/btm_dev.c:197:5: error: passing argument 1 of 'BTM_DeleteStoredLinkKey' from incompatible pointer type [-Werror]
     BTM_DeleteStoredLinkKey (&bda, NULL);
     ^
In file included from external/bluetooth/bluedroid/stack/./btm/btm_dev.c:34:0:
external/bluetooth/bluedroid/stack/include/btm_api.h:4411:32: note: expected 'UINT8 *' but argument is of type 'UINT8 (*)[6]'
     BTM_API extern tBTM_STATUS BTM_DeleteStoredLinkKey(BD_ADDR bd_addr, tBTM_CMPL_CB *p_cb);
                                ^

My main concern meanwhile is, that I don't really understand, what 'RawAddress' really does and what is passed on to the follow-on functions. So when backporting this, I am not sure whether the follow-on functions expect a pointer to the 1st array-element or the entire array. Further looking at the original patch, I am struggling to understand the logic at all - before the patch, the "device representation" should be deleted, now - a copy of that is passed for deletion (so the 'original' remains untouched) and the code contains warnings about invalidations...

I am almost at the point to say, don't apply that back port - but as indicated, my C / C++ knowledge is limited. So if you with your deeper C / C++ knowledge understand the logic behind, you will most probably have the right idea on how to proceed...?

hahnjo commented 6 years ago

Ah, there's the catch and the reason why I wasn't able to find the upstream patch (looked at the old repo).

I think the problem applies to the old code as well, and my guess should have been right: You need to pass the array (the address of its first element, that's the same) and not the address of the array. For illustration purpose:

typedef int addr[8];

void foo(addr a);

void bar() {
  addr a;
  foo(a);
  // foo(&a); // expected ‘int *’ but argument is of type ‘int (*)[8]’
  foo(&a[0]);
}

So change BTM_DeleteStoredLinkKey (&bda, NULL); to BTM_DeleteStoredLinkKey (bda, NULL); and it should work. The addressof-operator is needed in C++ because it's passing the location of a member in the object.

For completeness: RawAddress is now a class in C++ which has a constructor RawAddress::RawAddress(const uint8_t (&addr)[6]). This performs a copy of the passed array, the same operation that you are now performing with a call to memcpy.

MSe1969 commented 6 years ago

Thanks @hahnjo - it compiled now, will test later when I am done with all remaining patches. What is your thought about my approch with regards to CVE-2018-9466, CVE-2018-9467, CVE-2018-9472 (see above)?

MSe1969 commented 6 years ago

CVE-2018-11281 deferred, would require significant further backports

MSe1969 commented 6 years ago

Ready to test now - all uploaded, final test build running, will then flash and test. Focal areas for testing:

MSe1969 commented 6 years ago

Btw, seems that CVE-2018-9475 is relevant - will analyze tomorrow

MSe1969 commented 6 years ago

Found and added/implemented CVE-2018-9475 and CVE-2018-11261

MSe1969 commented 6 years ago

Tested, looks good. From my point of view, ready to ship. Thoughts?

hahnjo commented 6 years ago

@MSe1969 sorry, didn't have time for a detailed look. Approach for libxml2 sounds ok, the project was meant to be a "best effort".

MSe1969 commented 6 years ago

OK, thanks - I'll ship it, then

MSe1969 commented 6 years ago

Shipped, stuff merged into stable branch - closing this issue now