这段代码是否容易受到缓冲区溢出攻击？

Question

Fortify报告了下面代码中的缓冲区溢出漏洞，理由如下 -

在这种情况下，我们主要关注“取决于在代码的直接范围之外执行的数据属性”，因为我们无法验证abc.cpp中memcpy()执行的操作的安全性。

void create_dir(const char *sys_tmp_dir,  const char *base_name,
                size_t     base_name_len)
{
    char        *tmp_dir;
    size_t      sys_tmp_dir_len;

    sys_tmp_dir_len = strlen(sys_tmp_dir);

   tmp_dir = (char*) malloc(sys_tmp_dir_len + 1 + base_name_len + 1);
   if(NULL == tmp_dir) 
     return;

memcpy(tmp_dir, sys_tmp_dir, sys_tmp_dir_len);
tmp_dir[sys_tmp_dir_len] = FN_LIBCHAR;
memcpy(tmp_dir + sys_tmp_dir_len + 1, base_name, base_name_len);
tmp_dir[sys_tmp_dir_len + base_name_len + 1] = '\0';
..........
..........

}

我认为这是一个误报，因为我们首先获取数据的大小，然后分配相应的空间，接着调用memcpy函数进行复制。但是我正在寻找说服其他开发人员放弃当前实现，改用C++字符串的好理由。这个问题已经交给他了。他只是认为这是一个误报，所以不想改变任何东西。

编辑：我看到了对当前代码的快速有效批评。希望现在我能说服他。否则，我将接过这个问题。

- irsis

---

我猜测警告是因为你依赖于base_name_len的真实性。正如它所说，这在此代码范围内没有得到执行。使用更明智的东西，比如std::string就可以解决这个问题。 - Mike Seymour

除非有理由认为base_name不是以空字符结尾的字符串，否则您可以使用memcpy(tmp_dir + sys_tmp_dir_len + 1, base_name, base_name_len + 1);，这样memcpy()就会在字符串末尾添加空字符。如果base_name不准确或不是以空字符结尾的字符串，则您现有的代码更安全。如果您检查了malloc()的结果，我认为不存在缓冲区溢出的问题，但如果您认为无法信任传递的字符串，则应该检查它们是否为空。否则，您无法以可靠、可移植的方式检查它们的有效性。 - Jonathan Leffler

你有8行丑陋的代码，仍然需要一个free函数，但你可以用std::string一行代码替换它，在此过程中使其具有异常安全性，并且你知道它将消除工具中的误报 - 你还需要更多理由吗？如果你想使用C语言，可以使用asprintf()函数来满足你的可移植性需求...简洁的代码可以隐藏更少的错误并减少不必要的逻辑混淆。 - Tony Delroy

@MikeSeymour 是的，那是我的猜测。我知道使用std::string将有助于首先摆脱这段代码。Adriano在下面的评论对我很有意义。 - irsis

有趣的是，在短短20分钟内就已经出现了3个有效的批评（Mike Syemour对base_name_len的评论，Adriano Repetti对信任sys_tmp_dir的评论以及Matt对malloc失败的评论）。 - MSalters

@MSalters 是的！可悲的是，被指派修复问题的开发人员并不准备重构代码。他只是把这个看作是误报。 - irsis

Answer 1

看一下strlen()，它有输入字符串但没有上限，所以它会一直搜索直到找到\0。这是一个漏洞，因为您将依赖其结果执行memcpy()（如果它在搜索时不因访问违规而崩溃）。想象一下：

create_dir((const char*)12345, baseDir, strlen(baseDir));

你同时标记了C和C++...如果你在使用C++，那么std::string将会保护你免受这些问题的困扰。

- Adriano Repetti

---

没错，这是另一个漏洞。这会导致缓冲区溢出吗？你会如何称呼这个新漏洞？ - irsis

我不明白strlen()如何导致缓冲区溢出。它可以返回一个非常大的数字，这可能会导致malloc()失败并导致分段错误，但是如果分配的缓冲区大小没有失败，它将始终足够大以容纳写入。话虽如此，如果是C++代码，我肯定会使用std::string，如果是C代码，我会使用asfprintf()。就其编写而言，它让我感到头痛--对于这个任务来说过于复杂。（我注意到原始问题现在已经编辑添加了对malloc()的NULL测试--我仍然坚持过于复杂的说法） - K Scott Piel

假设输入字符串指针不指向字符串而是代码段，会发生什么？如果指针是故意留下的垃圾呢？结果取决于操作系统（和你的鼻子里的守护程序）。访问冲突、陷阱…… 读取与写入一样（可能）不安全，因为它可能导致在其他地方写入。如果在驱动程序中执行此操作，则情况会更糟。 - Adriano Repetti

Answer 2

在我的看法中，这是一个误报，因为我们首先获取数据的大小，然后分配相应的空间。

这种假设是与警告/错误匹配的问题。在您的代码中，您“假定”`malloc`成功分配了所请求的内存。如果您的系统没有多余的内存，`malloc`将失败并返回`NULL`。当您尝试将`memcpy`复制到`tmp_dir`时，您将复制到`NULL`，这将是个坏消息。

您应该在将其视为有效指针之前检查`malloc`返回的值是否为`NULL`。

- Matt K

---

谢谢。那个NULL检查已经在代码中添加了。我忘记加上它了。如果内存分配失败，现在只需返回“”。 - irsis