Comment 45 for bug 1476662

Revision history for this message
Roman Fiedler (roman-fiedler-deactivatedaccount) wrote :

Also to me. If I understand correctly, code inside e.g. validate_symlink is only secure when applied to a non-running container (where any process completely under guest control is launched). Otherwise there is TOCTOU between readlink and openat.

As the code is inside src/lxc/utils.c should we place comments before each of those functions, so that no one else will find them convenient, does not know about this implicit requirements and use it for something else.

"CAVEAT: This function must not be used for other purposes than container setup before executing the container's init?"

Apart from that, "validate_symlink" using destbuf would work around that but could make the code more prone to endless recursions. (Was the patch already checked against symlink loops?)

Could the description of open_without_symlink be out of sync with code?
"Open a path intending for mounting, ensuring there are no symbolic links in the path anywhere after 'prefix_skip' (if set)."
After checking the prefix, it calls open_if_safe which will validate_symlink and use it, when symlink encountered?

I haven't checked the whole patch yet, would take hours, but perhaps someone could do two straces of container startup on patched machine (one normal and one with a problematic symlink) and I will try to do the same analysis as for the initial report according to methods from https://service.ait.ac.at/security/2015/LxcSecurityAnalysis.html