search

yaml / libyaml

Canonical source repository for LibYAML
http://pyyaml.org/wiki/LibYAML
MIT License
921 stars 312 forks source link

Fix emitter states handling when write_indicator fails #290

Closed perlpunk closed 3 months ago

perlpunk commented 3 months ago

There are cases where yaml_emitter_write_indicator fails. In that case POP is called on emitter->indents but not on emitter->states, which results in a leftover event in the stack, and later POP is called on an empty emitter->indents stack.

This commit does not fix the case of the failing yaml_emitter_write_indicator. This is still investigated.

This will mitigate CVE-2024-3205

perlpunk commented 3 months ago

apparently some tests are failing, so there are cases where this is wrong... looking...

edit: fixed syntax issue

zhuofeng6 commented 3 months ago

That's great. i think it is a more general fix for this. So can we merge it?

perlpunk commented 3 months ago

Probably the emitter->state = POP(emitter, emitter->states); can be done directly after the POP on indents, instead of 3 times in different places. I don't know why @xitology put it in that order. I can't imagine a case where it would be correct to only pop from indents.

perlpunk commented 3 months ago

I moved the POP now directly after the indents POP statement. That should have the same effect.

ingydotnet commented 3 months ago

I'm currently extremely busy but I'll try to review this soon.

zhuofeng6 commented 3 months ago

This is a high score cve, which has a great impact on the libyaml community. If possible, it is necessary to merge this pr as soon as possible. This is my humble opinion

frenzymadness commented 3 months ago

The CVSS score for this vulnerability says that the attack complexity is low and the attract vector is the network (which means almost anybody can use this vulnerability to attack a system via a network). From the discussions here, I have a very different feeling. Even the maintainers of the software itself have a hard time exploiting/reproducing the vulnerability. It's either easy to reproduce or the score and therefore the severity of the vulnerability is wrong.

perlpunk commented 3 months ago

Note that the reproducers I found use the canonical mode, which is probably rarely used. But I think it could be possible to produce the same effect without canonical mode, and possibly someone already knows the necessary input for that.

perlpunk commented 3 months ago

Please see my update that I don't consider it a vulnerability: https://github.com/yaml/libyaml/issues/258#issuecomment-2058613931

zhuofeng6 commented 3 months ago

I agree with you. I don't think this is a cve either. but now does this PR still need to be merged?

perlpunk commented 3 months ago

I think the PR an improvement, but I would rather try to check in yaml_emitter_close if the emitter is in an error state. probably I can just check if an error was set. just very busy in the middle of two conferences, and it's not urgent IMHO

perlpunk commented 3 months ago

I will create a different PR when I have time