Project

General

Profile

Actions

Bug #2514

closed

Unsafe use of getenv("HOME") in SecPublicInfoSqlite3 and SecTpmFile

Added by Alex Afanasyev about 9 years ago. Updated almost 8 years ago.

Status:
Closed
Priority:
Normal
Category:
Security
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

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
Actions #1

Updated by Joao Pereira almost 9 years ago

I have 3 suggestions to solve this issue

  1. Use a MACRO that will call the std::getenv("HOME")
  2. Add a function in common.hpp or another file that we see feat, to call std::getenv and see if the result is correct
  3. 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

Actions #2

Updated by Junxiao Shi almost 9 years ago

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")
Actions #3

Updated by Joao Pereira almost 9 years ago

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")

Actions #4

Updated by Junxiao Shi almost 9 years ago

What about:

std::string
getHomeDirectory();

But this shouldn't appear in common.hpp because it's not generic enough.

Actions #5

Updated by Joao Pereira almost 9 years ago

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?

Actions #6

Updated by Junxiao Shi almost 9 years ago

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.

Actions #7

Updated by Joao Pereira almost 9 years ago

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

Actions #8

Updated by Alex Afanasyev almost 8 years ago

  • Assignee set to Alex Afanasyev
  • % Done changed from 0 to 100
Actions #9

Updated by Alex Afanasyev almost 8 years ago

  • Status changed from New to Code review
Actions #10

Updated by Alex Afanasyev almost 8 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF