double-free on yaml_event_delete api

[idhyt]

Description

alloc anchor by api yaml_sequence_start_event_initialize, then add event by api yaml_emitter_emit,
we could free anchor again by yaml_event_delete after call yaml_emitter_delete
the detail process in below:

    step1: yaml_sequence_start_event_initialize
            -> alloc `anchor` by yaml_strdup at api.c:887
    step2: yaml_emitter_initialize && yaml_emitter_emit
            -> add event, call ENQUEUE (emitter.c:288) copy data from event to emitter.events.tail, (*((queue).tail++) = value, 1)
    step3: yaml_emitter_delete
            -> first free at api.c:400 -> yaml_event_delete(&DEQUEUE(emitter, emitter->events))
    step4: yaml_event_delete
            -> double free at api.c:1015

ASAN Report

╰─ ./poc
double-free poc
=================================================================
==1897917==ERROR: AddressSanitizer: attempting double-free on 0x602000000010 in thread T0:
    #0 0x4937bd in free (~/idhyt/poc/poc+0x4937bd)
    #1 0x4c5bd7 in yaml_free ~/idhyt/poc/libyaml/src/api.c:53:14
    #2 0x4c5bd7 in yaml_event_delete ~/idhyt/poc/libyaml/src/api.c:1015:13
    #3 0x4c335f in poc ~/idhyt/poc/./double-free.c:33:5
    #4 0x4c343f in main ~/idhyt/poc/./double-free.c:41:5
    #5 0x7f3803d7a082 in __libc_start_main /build/glibc-e2p3jK/glibc-2.31/csu/../csu/libc-start.c:308:16
    #6 0x41b2fd in _start (~/idhyt/poc/poc+0x41b2fd)

0x602000000010 is located 0 bytes inside of 7-byte region [0x602000000010,0x602000000017)
freed by thread T0 here:
    #0 0x4937bd in free (~/idhyt/poc/poc+0x4937bd)
    #1 0x4c5bd7 in yaml_free ~/idhyt/poc/libyaml/src/api.c:53:14
    #2 0x4c5bd7 in yaml_event_delete ~/idhyt/poc/libyaml/src/api.c:1015:13
    #3 0x61b0000007e7  (<unknown module>)

previously allocated by thread T0 here:
    #0 0x47fde4 in strdup (~/idhyt/poc/poc+0x47fde4)
    #1 0x4c79aa in yaml_strdup ~/idhyt/poc/libyaml/src/api.c:66:27
    #2 0x4c79aa in yaml_sequence_start_event_initialize ~/idhyt/poc/libyaml/src/api.c:887:23
    #3 0x4c32f9 in poc ~/idhyt/poc/./double-free.c:24:5
    #4 0x4c343f in main ~/idhyt/poc/./double-free.c:41:5

SUMMARY: AddressSanitizer: double-free (~/idhyt/poc/poc+0x4937bd) in free
==1897917==ABORTING

Poc

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <yaml.h>

void poc() {
    yaml_document_t document;
    memset(&document, 0, sizeof(yaml_document_t));
    yaml_document_initialize(&document, NULL, NULL, NULL, 0, 0);

    yaml_event_t event;
    memset(&event, 0, sizeof(yaml_event_t));
    int encoding = YAML_ANY_ENCODING;

    yaml_document_add_sequence(&document, YAML_NULL_TAG, YAML_ANY_SEQUENCE_STYLE);

    // step1: allocated by yaml_strdup(anchor) at api.c:887
    yaml_sequence_start_event_initialize(&event, "anchor", YAML_NULL_TAG, 0, YAML_ANY_SEQUENCE_STYLE);

    yaml_emitter_t emitter;
    memset(&emitter, 0, sizeof(yaml_emitter_t));
    yaml_emitter_initialize(&emitter);

    // step2: yaml_emitter_emit call ENQUEUE (emitter.c:288) copy data from event to emitter.events.tail -> (*((queue).tail++) = value, 1)
    yaml_emitter_emit(&emitter, &event);
    // step3: first free at api.c:400 -> yaml_event_delete(&DEQUEUE(emitter, emitter->events));
    yaml_emitter_delete(&emitter);
    // step4: double free at api.c:1015
    yaml_event_delete(&event);

    yaml_document_delete(&document);
}

int main(int argc, char *argv[])
{
    printf("double-free poc\n");
    poc();
    return 0;
}

the reserved CVE ID CVE-2024-35325 will be made public after the vulnerability is fixed.

[idhyt]

No one pays attention，make all public 👾

[perlpunk]

I'm not a C expert, but calling yaml_event_delete after yaml_emitter_delete shouldn't be necessary.
How is that a vulnerability?
Do you have a suggestion to improve the code?
Btw, this is an unpaid hobby project. If this was paid, we would pay attention to such things earlier. Two weeks is quite short.
And, btw, this issue is already public.

[idhyt]

As you mentioned, yaml_emitter_delete should not be called, but you cann't ensure every developer will use your API as expected. software vulnerabilities are caused by unexpected user behavior.

Regarding the fix, my understanding is that the queue should store object pointers, the pointers can simply be set to NULL after free.

There are many places where references are implemented, with my limited understanding of the project, sorry unable to provide a complete fix or patch currently~

In addition, this project is a widely-used public library. I search the history vuln on snyk, the affected public components and systems exceed 200+.

No one pay attention in the last two weeks, I thought the project was deprected and no longer maintained~

[perlpunk]

As you mentioned, yaml_emitter_delete should not be called

I did not say that. I said yaml_event_delete.

Let me just quote the documentation for the emitter on https://pyyaml.org/wiki/LibYAML

#include <yaml.h>

yaml_emitter_t emitter;
yaml_event_t event;

/* Create the Emitter object. */
yaml_emitter_initialize(&emitter);

/* Set a file output. */
FILE *output = fopen("...", "wb");

yaml_emitter_set_output_file(&emitter, output);

/* Set a generic writer. */
void *ext = ...;
int write_handler(void *ext, char *buffer, int size) {
    /*
       ...
       Write `size` bytes.
       ...
    */
    return error ? 0 : 1;
}

yaml_emitter_set_output(&emitter, write_handler, ext);

/* Create and emit the STREAM-START event. */
yaml_stream_start_event_initialize(&event, YAML_UTF8_ENCODING);
if (!yaml_emitter_emit(&emitter, &event))
    goto error;

/*
  ...
  Emit more events.
  ...
*/

/* Create and emit the STREAM-END event. */
yaml_stream_end_event_initialize(&event);
if (!yaml_emitter_emit(&emitter, &event))
    goto error;

/* Destroy the Emitter object. */
yaml_emitter_delete(&emitter);

return 1;

/* On error. */
error:

/* Destroy the Emitter object. */
yaml_emitter_delete(emitter);

return 0;

It doesn't even mention yaml_event_delete. Only in the parser code.

I can also not prevent people from calling free twice. It's C.

If there is really code out there that looks like yours, it would double free all the time whenever there are anchors or tags involved, so people's error logs should be full and they would hopefully look into their logs.

It might be possible to be more defensive by using pointers, you said, but that sounds like a big change that any binding would have to adjust to as well. I personally don't have any time for that in the foreseeable future.

No one pay attention in the last two weeks, I thought the project was deprected and no longer maintained~

Oh come on. The last commit is three weeks old, and additionally it is an old library that isn't expected to get much changes anyway.

[perlpunk]

Was this #298 CVE-2024-35329 created by you?
We didn't even get a security report or issue here about that, how unfriendly is that?

[Martchus]

@idhyt

… but you cann't ensure every developer will use your API as expected.

In general, the code of those developers should get a CVE filed then.

In this case I think there is indeed room for improvement, though. The code mentioned in the issue description is something that supposedly should be supported. The symmetrical invocations of …_initialize and _…delete are just something very natural and not supporting it is really a pitfall.

Regarding the fix, my understanding is that the queue should store object pointers, the pointers can simply be set to NULL after free.

That sounds reasonable (although I am not familiar with the codebase so I can't say for sure). So you may go ahead and create a PR implementing/demonstrating your idea.

In addition, this project is a widely-used public library.

That doesn't mean the library has many contributors, though.

No one pay attention in the last two weeks, I thought the project was deprected and no longer maintained~

Pressuring developers of open source projects this way was one of the pieces that allowed the recent xz incident to happen…

---

@perlpunk

If there is really code out there that looks like yours, it would double free all the time whenever there are anchors or tags involved, so people's error logs should be full and they would hopefully look into their logs.

Unfortunately, there will normally be no logs on a double-free. The output from the issue description only has those logged because an address sanitizer was used but that's normally not the case.

[idhyt]

@perlpunk

yaml_emitter_delete in description is indeed my typo error(yaml_event_delete), don't worry about it~

Thank you very much for your detailed explanation and sorry again for the trouble this has caused you~

maybe there is a difference in our understanding of vulnerability.

The last commit is three weeks old...

the latest version is v0.2.5 at 2020y，It is also the version introduced by many open source libraries and systems，Can you ensure that everyone who use this project will pull the latest code for compilation?

about unfriendly, As I said, no one paid attention to this issue in the past two weeks, so I made cve public and uploaded part of the pocs，If you think this method is unfriendly, I will delete all information and you can reject all cve，As of receipt of your reply, I have terminated all remaining reports.

[idhyt]

@Martchus

Pressuring developers of open source projects this way was one of the pieces that allowed the recent xz incident to happen…

Thank you for your kind reminder. This is indeed something I hadn't considered.

I used to be a security researcher, and although I haven't been hunting for vulnerabilities for many years, I would like to share the background of this issue.

Initially, during my development process with Rust, I needed to parse YAML files, so I introduced the serde_yaml library from Rust. When compiling, I discovered that it depended on unsafe-libyaml.

In line with my habit of being cautious about the use of unsafe in Rust development, I eventually found this project and noticed that the last released version was four years ago. I roughly reviewed the API implementation and discovered some issues, which I reproduced in unsafe-libyaml as shown in the poc.

I evaluated the impact of this library and found that it is indeed widely used, and no new version has been released recently. Ultimately, I decided to abandon using this library and switched to the serde_toml library.

From the perspective of a security practitioner, or from different developers' perspectives, the tolerance for code risks varies, as does the understanding of vulnerabilities.

If there is no consensus on the definition of a vulnerability (risks), then further discussion may not be very meaningful.

[perlpunk]

about unfriendly, As I said, no one paid attention to this issue in the past two weeks, so I made cve public and uploaded part of the pocs，If you think this method is unfriendly, I will delete all information and you can reject all cve，As of receipt of your reply, I have terminated all remaining reports.

You created this issue here about CVE-2024-35325
But what you published instead was CVE-2024-35329 as noted in #298
And for that we didn't get any report from you before disclosure.

[idhyt]

No one pays attention，make all public 👾

Maybe I didn't express it clearly, all public. The 4 public POCs could find in this repo(you can reject them). also I have terminated all remaining reports.

[perlpunk]

But you only reported this issue for one of the four. and then published another one of the four.
Please read what I wrote.

This issue is about CVE-2024-35325
That is a 5 at the end.
But that one is not published yet:
https://nvd.nist.gov/vuln/detail/CVE-2024-35325

Instead you took CVE-2024-35329 (that is a 9 at the end) and published that one:
https://nvd.nist.gov/vuln/detail/CVE-2024-35329
without notifying us before that.
You confused numbers. That happens, but it would be nice if you actually admitted that you made a mistake.

[perlpunk]

And as I said before, this issue was already public when you created it. IT's not a private issue.
There is a https://github.com/yaml/libyaml/security link at the top where you can create a private security report.
If you were a security researcher in the past like you stated, you should know to be a bit more careful with this.

[idhyt]

I have explained it very clearly. I made all public. I can't decide the order in which nist should be made public.

In addition, if the discussion is not about the vulnerability or you don't think this is a vulnerability, you just reject it and close this issue.

I should sleep now，Sorry for taking up so much of your time~

[perlpunk]

No you didn't explain very clearly. You only reported one of the 4 issues to us.
Disclosure before notifying is a no go!
Closing.