Fix some UAF errors report in #4214 - #4228
Conversation
|
Some builds are red.
|
`ber_sockbuf_add_io` only stores the raw pointer to Sockbuf_IO rather than copying the structure contents. So the sbio object needs to remain valid and outlive the sockbuf I/O layer. See it in: https://github.com/delphij/openldap/blob/master/libraries/liblber/sockbuf.c#L201 Cleanup requires calling `ber_sockbuf_remove_io` and is relatively complex, this is not implemented.
`gpgme_data_new_from_cbs` only stores the raw pointer to `gpgme_data_cbs_t` rather than copying the structure contents. So the cbs object needs to remain valid. See it in: https://github.com/gpg/gpgme/blob/master/src/data-user.c#L99 Cleanup requires is relatively complex, this is not implemented.
0cea47b to
dfc542b
Compare
| if(io) { | ||
| #define GO(A) my_io.A = find_##A##_Fct(io->A) | ||
| // TODO: memory leak, my_io should be free in ber_sockbuf_remove_io | ||
| my_io = (my_sockbuf_io_t*)box_calloc(1, sizeof(my_sockbuf_io_t)); |
There was a problem hiding this comment.
I don't want a solution that explicitely create a memory leak. Maybe a find_XXXX based on io address would be a better solution.
There was a problem hiding this comment.
I am still unfamiliar with how find_xxx works; I will look into it first.
| cbs_.seek = find_seek_Fct(cbs->seek); | ||
| cbs_.release = find_release_Fct(cbs->release); | ||
| // TODO: memory leak, cbs_ should be free in my_gpgme_data_release. | ||
| cbs_ = box_calloc(1, sizeof(my_gpgme_data_cbs_t)); |
There was a problem hiding this comment.
Were is the free of this calloc? the Claude repport mentionned the possibility of using gpgme_data_release for that.
There was a problem hiding this comment.
As can be seen from https://github.com/gpg/gpgme/blob/master/src/data-user.c#L99, the cbs object is stored in gpgme_data->data.user.cbs. Therefore, the struct gpgme_data structure (defined at https://github.com/gpg/gpgme/blob/master/src/data.h#L135) needs to be referenced to retrieve the stored cbs.
I have added the my_gpgme_data_release to free cbs. Please help to review it again.
There was a problem hiding this comment.
There is something wrong here. The union is a simplified version of the actual structure (which is fine), but the issue is, those user cbs/handle fields might not be valid, for a data intialized with an fd for example. And I don't see any field that is setup to allow knowing wich field of the union is valid.
There was a problem hiding this comment.
There is something wrong here. The union is a simplified version of the actual structure (which is fine), but the issue is, those user cbs/handle fields might not be valid, for a data intialized with an fd for example. And I don't see any field that is setup to allow knowing wich field of the union is valid.
Getting the cbs via data.user.cbs may be an incorrect approach. I think we need to create a map between gpgme_data and cbs to get the cbs object to be freed. This seems like the correct direction. But this adds some complexity. What do you think?
There was a problem hiding this comment.
Either a map or the static find_XXXX approach. I really depend on the occurence of this call
dfc542b to
0ad2623
Compare
| { | ||
| struct my_gpgme_data *gpgme_data = (struct my_gpgme_data *)data; | ||
| if (gpgme_data) { | ||
| my_gpgme_data_cbs_t *cbs_ = gpgme_data->data.user.cbs; |
There was a problem hiding this comment.
According to the gpgme sources, the cbs struct is the 1st argument of the structure, which, in that case, don't even need to be defined.
There was a problem hiding this comment.
There are two cbs objects inside gpgme_data. Looking at the source code of gpgme_data_new_from_cbs, it uses the latter one.
0ad2623 to
dfc542b
Compare
2 participants
Both
my_gpgme_data_new_from_cbsandmy_ber_sockbuf_add_ioare supplied with stack-allocated variables. These variables go out of live scope once the function returns and must not be accessed afterwards.Set it in: