Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problems in libyaml fuzzer programs #11811

Open
perlpunk opened this issue Apr 16, 2024 · 1 comment
Open

Problems in libyaml fuzzer programs #11811

perlpunk opened this issue Apr 16, 2024 · 1 comment

Comments

@perlpunk
Copy link
Contributor

This is about https://nvd.nist.gov/vuln/detail/CVE-2024-3205

When trying to reproduce it, I wasn't able to get the same behaviour when using a normal buffer. I only saw it when using the fuzzer code.

I think there are two mistakes in the libyaml fuzzers.
One only applies to the libyaml_dumper_fuzzer, the other is about the write_handler used in the other programs as well.

Please see my detailed explanation in yaml/libyaml#258 (comment)

Because of this, we are rejecting the CVE.

I would make a PR but am not sure about the expected behaviour.

perlpunk added a commit to perlpunk/oss-fuzz that referenced this issue Apr 18, 2024
See google#11811

libyaml expects 1 for success and 0 for failure.

https://github.com/yaml/libyaml/blob/master/src/writer.c#L53-L62

    if (emitter->write_handler(emitter->write_handler_data, ...) {
        ...
    }
    else {
        return yaml_emitter_set_writer_error(emitter, "write error");
    }

The logic is currently reverse, which (in combination of the failing
check for the yaml_emitter_dump return value) caused several wrong bug reports
and a CVE.

The fuzzer programs just ignore the failing yaml_emitter_dump, and so I
assume it never appeared as a problem. Only in the cases where the wrongly
called yaml_emitter_close ran into a case where it popped from an empty
stack an overflow was detected.

The input YAML in question just had a lot of nested sequences in the form

    - - - - -

which in canonical output mode resulted in a large output because of the
indentation, and so the buffer flush was triggered before the emitter
finished:
!!seq [
  !!seq [
    ...

In the most cases the YAML is simply too small to produce the error because
the flush happened when the output was complete.
@perlpunk
Copy link
Contributor Author

I created a PR to fix one of the problems:

DonggeLiu pushed a commit that referenced this issue Apr 19, 2024
See #11811

libyaml expects 1 for success and 0 for failure.

https://github.com/yaml/libyaml/blob/master/src/writer.c#L53-L62

    if (emitter->write_handler(emitter->write_handler_data, ...) {
        ...
    }
    else {
        return yaml_emitter_set_writer_error(emitter, "write error");
    }

The logic is currently reverse, which (in combination of the failing
check for the yaml_emitter_dump return value) caused several wrong bug
reports and a CVE.

The fuzzer programs just ignore the failing yaml_emitter_dump, and so I
assume it never appeared as a problem. Only in the cases where the
wrongly called yaml_emitter_close ran into a case where it popped from
an empty stack an overflow was detected.

The input YAML in question just had a lot of nested sequences in the
form

    - - - - -

which in canonical output mode resulted in a large output because of the
indentation, and so the buffer flush was triggered before the emitter
finished:

    !!seq [
      !!seq [
        ...

In the most cases the YAML is simply too small to produce the error
because the flush happened when the output was complete.

Note that this does not yet fix the missing error handling of
`yaml_emitter_dump` in libyaml_dumper_fuzzer.c etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant