Re: [cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
Once a command reports an error CMake will not generate the project so it is not worth allowing the configuration to do much after that. Failure of file(DOWNLOAD) should either be a cmake::FATAL_ERROR or just a STATUS setting with no CMake Error. The signature needs a way for CMake to know which one to do. I'm fine with changing the current non-fatal error to a fatal error in the next release. Would this be: (1) an unconditional, always-in-effect change? (2) a policy with a NEW behavior? (3) a change activated only by use of a new keyword argument to file(DOWNLOAD ? File download errors are quite common for many different reasons I'm not sure I like the idea of triggering a CMake fatal error for a corrupt download. File download should mostly be a build-time step in my opinion. Configure should be as fast as possible, meaning: only do the minimum try_compiles necessary, and defer until build time things that take a long time, like download over a network. ? D -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
Read the rest of the thread in more detail. We propose adding an additional signature to allow scripts to avoid a CMake Error and handle the failure themselves. In either case we should not have a non-fatal error. OK -- sorry for the distraction... I should have looked at the code first. D -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
On 11/21/2013 10:46 AM, David Cole wrote: >> I'm fine with changing the current non-fatal error to a fatal error >> in the next release. > > Would this be: > (1) an unconditional, always-in-effect change? > (2) a policy with a NEW behavior? > (3) a change activated only by use of a new keyword argument to > file(DOWNLOAD ? > > File download errors are quite common for many different reasons > I'm not sure I like the idea of triggering a CMake fatal error for a > corrupt download. Even without this change it is already an error. If it is used in a build-time script the script will exit with an error when finished. If it is in a CMakeLists.txt file, CMake will not generate the project when finished. The concern is that in either case the caller of file(DOWNLOAD) may immediately do something with the downloaded content. By using EXPECTED_HASH, a caller says it expects very specific content. If it does not match we should not let the caller proceed to do something with unexpected content. Therefore option (1) that you list above is the best fix. Read the rest of the thread in more detail. We propose adding an additional signature to allow scripts to avoid a CMake Error and handle the failure themselves. In either case we should not have a non-fatal error. -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
On 11/20/2013 04:05 AM, Daniele E. Domenichelli wrote: >> The "this->SetError/return false" logic for these errors should be >> replaced by "this->IssueMessage(cmake::FATAL_ERROR,...)/return true" >> to switch it to a fatal error. The signature should be extended >> to provide an option to get the error information back without >> causing a CMake Error so that the caller can handle it. > > What about setting the STATUS variable to > "some number different from 0; check failed" instead? > In this way the default behaviour won't change and there is no need to > extend the signature, but if you check the STATUS variable, you will be > able to issue a fatal error. > Also if download fails in some other way, the error raised is not fatal, > therefore in this way it looks more coherent. Once a command reports an error CMake will not generate the project so it is not worth allowing the configuration to do much after that. Failure of file(DOWNLOAD) should either be a cmake::FATAL_ERROR or just a STATUS setting with no CMake Error. The signature needs a way for CMake to know which one to do. I'm fine with changing the current non-fatal error to a fatal error in the next release. -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
On 19/11/13 16:34, Brad King wrote: >> * The "STATUS" variable is not set, therefore it is not useful; >> * The "faulty" downloaded file is not removed. >> >> So I believe that there is no way to stop CMake, unless you perform >> another hash check. > > The "this->SetError/return false" logic for these errors should be > replaced by "this->IssueMessage(cmake::FATAL_ERROR,...)/return true" > to switch it to a fatal error. The signature should be extended > to provide an option to get the error information back without > causing a CMake Error so that the caller can handle it. What about setting the STATUS variable to "some number different from 0; check failed" instead? In this way the default behaviour won't change and there is no need to extend the signature, but if you check the STATUS variable, you will be able to issue a fatal error. Also if download fails in some other way, the error raised is not fatal, therefore in this way it looks more coherent. Daniele -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
Re: [cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
On 11/19/2013 10:24 AM, Daniele E. Domenichelli wrote: > After calling file(DOWNLOAD EXPECTED_HASH) I cannot find a way to check > if the hash is correct. > > * The command gives an error, but not fatal, therefore the processing > will continue; IIRC the use case for which this was built put file(DOWNLOAD) inside a script invoked with "cmake -P" (see ExternalProject). That will still exit with a bad code and the caller will see it. > * The "STATUS" variable is not set, therefore it is not useful; > * The "faulty" downloaded file is not removed. > > So I believe that there is no way to stop CMake, unless you perform > another hash check. The "this->SetError/return false" logic for these errors should be replaced by "this->IssueMessage(cmake::FATAL_ERROR,...)/return true" to switch it to a fatal error. The signature should be extended to provide an option to get the error information back without causing a CMake Error so that the caller can handle it. > I suggest to fix this as soon as possible (perhaps even in the 2.8 > series), either failing with a fatal error or setting the STATUS > variable. What do you think? The feature was originally built as EXPECTED_MD5 which is definitely *not a security check* but rather a corruption check. It was then generalized to EXPECTED_HASH just because other hashes algorithms are now available, so only then did it become useful for security. The above fix is a change in behavior which IMO does not belong in a tweak release. -Brad -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
[cmake-developers] file(DOWNLOAD) + EXPECTED_HASH security issue
Hello all, After calling file(DOWNLOAD EXPECTED_HASH) I cannot find a way to check if the hash is correct. * The command gives an error, but not fatal, therefore the processing will continue; * The "STATUS" variable is not set, therefore it is not useful; * The "faulty" downloaded file is not removed. So I believe that there is no way to stop CMake, unless you perform another hash check. Am I missing something? I believe this could be a potential security issue on some build systems, because after using the file(DOWNLOAD) command with EXPECTED_HASH, users might be thinking that the following code will not be executed if the hash is wrong, or they might be checking the STATUS variable, expecting it to be set != 0 if the hash is wrong (as I was expecting by reading the documentation of the FILE command). Therefore they might be using the downloaded file during the same CMake execution, and this might be exploited by some attacker. I suggest to fix this as soon as possible (perhaps even in the 2.8 series), either failing with a fatal error or setting the STATUS variable. What do you think? Cheers, Daniele -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
