Closed idhyt closed 1 month ago
No one pays attention,make all public 👾
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.
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~
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.
Was this https://github.com/yaml/libyaml/issues/298 CVE-2024-35329 created by you? We didn't even get a security report or issue here about that, how unfriendly is that?
@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.
@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.
@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.
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 https://github.com/yaml/libyaml/issues/298 And for that we didn't get any report from you before disclosure.
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.
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.
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.
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~
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.
Description
alloc
anchor
by apiyaml_sequence_start_event_initialize
, then add event by apiyaml_emitter_emit
, we could freeanchor
again byyaml_event_delete
after callyaml_emitter_delete
the detail process in below:ASAN Report
Poc
the reserved CVE ID
CVE-2024-35325
will be made public after the vulnerability is fixed.