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

libyaml: Fix yaml_write_handler return values #11818

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Apr 18, 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.

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.
Copy link

google-cla bot commented Apr 18, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

perlpunk is a new contributor to projects/libyaml. The PR must be approved by known contributors before it can be merged. The past contributors are: ingydotnet, alex, rjotwani, devtty1er, Dor1s, ssbr (unverified), sigmavirus24 (unverified), inferno-chromium (unverified), mikea (unverified)

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

Successfully merging this pull request may close these issues.

None yet

3 participants