Bug #2514
closed
Unsafe use of getenv("HOME") in SecPublicInfoSqlite3 and SecTpmFile
Added by Alex Afanasyev almost 10 years ago.
Updated over 8 years ago.
Description
Not in all environments, HOME variable is defined. In cases when it is not, the current code results in obscure error. For example, my initial attempt on Android resulted in the following error:
02-12 00:18:08.374: A/NfdWrapper(2378): basic_string::_S_construct null not valid
I have 3 suggestions to solve this issue
- Use a MACRO that will call the
std::getenv("HOME")
- Add a function in common.hpp or another file that we see feat, to call
std::getenv
and see if the result is correct
- Override
std::getenv
adding a default value
namespace std{
string getenv(string variableName, string defaultValue);
}
I like the third option, but not sure if it is nice to be messing around with std functions. Maybe we can do something similar in option 2
I disagree with all solutions in note-1.
std::stdenv
shouldn't be altered.
This problem occurs only in SecPublicInfoSqlite3
and SecTpmFile
.
These two classes, after calling std::getenv("HOME")
, should check whether the return value equals nullptr
.
std::getenv("HOME") == nullptr ? "/root" : std::getenv("HOME")
I thought of centralizing that, because I really do not like to see hardcoded things around, specially paths.
Think that at least the path should be centralized somewhere in a const or something.
const std::string nfd::defaultPath("/root");
and change the code in both places to:
std::getenv("HOME") == nullptr ? nfd::defaultPath : std::getenv("HOME")
What about:
std::string
getHomeDirectory();
But this shouldn't appear in common.hpp
because it's not generic enough.
That was my second solution, but I am not sure where we can put this because it should be accessible in both the places. Is there a .hpp where we store all our generic functions?
I looked at util/config-file.cpp
which also uses std::getenv("HOME")
.
I feel it's incorrect to imply "/root"
or any other home directory when HOME
environ is unset.
Therefore, SecPublicInfoSqlite3
and SecTpmFile
could simply report an error when std::getenv("HOME")
equals nullptr.
What if we add a default value in the config file? It would be more flexible don't you think? We can assume /root as the default value for that parameter
- Assignee set to Alex Afanasyev
- % Done changed from 0 to 100
- Status changed from New to Code review
- Status changed from Code review to Closed
Also available in: Atom
PDF