Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
security fixes
  • Loading branch information
jjergus committed Nov 12, 2020
1 parent 5a43f24 commit abe0b29
Show file tree
Hide file tree
Showing 21 changed files with 185 additions and 44 deletions.
17 changes: 9 additions & 8 deletions hphp/runtime/base/preg.cpp
Expand Up @@ -49,6 +49,7 @@
#include "hphp/util/logger.h"
#include "hphp/util/concurrent-scalable-cache.h"

#include <folly/FileUtil.h>
#include <folly/json.h>

/* Only defined in pcre >= 8.32 */
Expand Down Expand Up @@ -217,7 +218,7 @@ struct PCRECache {
TempKeyCache& keyCache);
void insert(Accessor& accessor, const StringData* regex,
TempKeyCache& keyCache, const pcre_cache_entry* ent);
void dump(const std::string& filename);
void dump(folly::File& file);
size_t size() const;

private:
Expand Down Expand Up @@ -512,12 +513,12 @@ void PCRECache::insert(
}
}

void PCRECache::dump(const std::string& filename) {
std::ofstream out(filename.c_str());
void PCRECache::dump(folly::File& file) {
switch (m_kind) {
case CacheKind::Static:
for (auto& it : *m_staticCache) {
out << it.first->data() << "\n";
folly::writeFull(file.fd(), it.first->data(), it.first->size());
folly::writeFull(file.fd(), "\n", 1);
}
break;
case CacheKind::Lru:
Expand All @@ -530,12 +531,12 @@ void PCRECache::dump(const std::string& filename) {
m_scalableCache->snapshotKeys(keys);
}
for (auto& key: keys) {
out << key.c_str() << "\n";
folly::writeFull(file.fd(), key.data(), key.size());
folly::writeFull(file.fd(), "\n", 1);
}
}
break;
}
out.close();
}

size_t PCRECache::size() const {
Expand Down Expand Up @@ -572,8 +573,8 @@ void pcre_reinit() {
void pcre_init() {
}

void pcre_dump_cache(const std::string& filename) {
s_pcreCache.dump(filename);
void pcre_dump_cache(folly::File& file) {
s_pcreCache.dump(file);
}

static pcre_jit_stack* alloc_jit_stack(void* /*data*/) {
Expand Down
5 changes: 3 additions & 2 deletions hphp/runtime/base/preg.h
Expand Up @@ -18,6 +18,7 @@

#include "hphp/runtime/base/type-string.h"

#include <folly/File.h>
#include <folly/Optional.h>

#include <cstdint>
Expand Down Expand Up @@ -111,9 +112,9 @@ void pcre_reinit();
void pcre_session_exit();

/*
* Dump the contents of the PCRE cache to filename.
* Dump the contents of the PCRE cache to the given file.
*/
void pcre_dump_cache(const std::string& filename);
void pcre_dump_cache(folly::File& file);

///////////////////////////////////////////////////////////////////////////////
// PHP API
Expand Down
6 changes: 6 additions & 0 deletions hphp/runtime/base/runtime-option.cpp
Expand Up @@ -54,6 +54,7 @@
#include "hphp/util/gzip.h"
#include "hphp/util/hardware-counter.h"
#include "hphp/util/hdf.h"
#include "hphp/util/light-process.h"
#include "hphp/util/log-file-flusher.h"
#include "hphp/util/logger.h"
#include "hphp/util/network.h"
Expand Down Expand Up @@ -754,6 +755,7 @@ bool RuntimeOption::AdminServerStatsNeedPassword = true;
std::string RuntimeOption::AdminPassword;
std::set<std::string> RuntimeOption::AdminPasswords;
std::set<std::string> RuntimeOption::HashedAdminPasswords;
std::string RuntimeOption::AdminDumpPath;

std::string RuntimeOption::ProxyOriginRaw;
int RuntimeOption::ProxyPercentageRaw = 0;
Expand Down Expand Up @@ -2398,6 +2400,8 @@ void RuntimeOption::Load(
"Server.LightProcessFilePrefix", "./lightprocess");
Config::Bind(LightProcessCount, ini, config,
"Server.LightProcessCount", 0);
Config::Bind(LightProcess::g_strictUser, ini, config,
"Server.LightProcessStrictUser", false);
Config::Bind(ForceServerNameToHeader, ini, config,
"Server.ForceServerNameToHeader");
Config::Bind(AllowDuplicateCookies, ini, config,
Expand Down Expand Up @@ -2513,6 +2517,8 @@ void RuntimeOption::Load(
AdminPasswords = Config::GetSet(ini, config, "AdminServer.Passwords");
HashedAdminPasswords =
Config::GetSet(ini, config, "AdminServer.HashedPasswords");
Config::Bind(AdminDumpPath, ini, config,
"AdminServer.DumpPath", "/tmp/hhvm_admin_dump");
}
{
// Proxy
Expand Down
2 changes: 2 additions & 0 deletions hphp/runtime/base/runtime-option.h
Expand Up @@ -496,6 +496,8 @@ struct RuntimeOption {
static std::set<std::string> AdminPasswords;
static std::set<std::string> HashedAdminPasswords;

static std::string AdminDumpPath;

/*
* Options related to reverse proxying. ProxyOriginRaw and ProxyPercentageRaw
* may be mutated by background threads and should only be read or written
Expand Down
3 changes: 2 additions & 1 deletion hphp/runtime/base/string-util.cpp
Expand Up @@ -435,7 +435,8 @@ String StringUtil::ROT13(const String& input) {
}

String StringUtil::Crypt(const String& input, const char *salt /* = "" */) {
if (salt && salt[0] == '\0') {
assertx(salt);
if (salt[0] == '\0') {
raise_notice("crypt(): No salt parameter was specified."
" You must use a randomly generated salt and a strong"
" hash function to produce a secure hash.");
Expand Down
4 changes: 3 additions & 1 deletion hphp/runtime/ext/domdocument/ext_domdocument.h
Expand Up @@ -51,7 +51,9 @@ struct DOMNode {
// for __clone
DOMNode& operator=(const DOMNode& copy);

req::ptr<XMLDocumentData> doc() const { return m_node->doc(); }
req::ptr<XMLDocumentData> doc() const {
return m_node ? m_node->doc() : nullptr;
}
XMLNode node() const { return m_node; }
xmlNodePtr nodep() const {
return m_node ? m_node->nodep() : nullptr;
Expand Down
4 changes: 2 additions & 2 deletions hphp/runtime/ext/gd/libgd/gd.cpp
Expand Up @@ -185,8 +185,8 @@ gdImagePtr gdImageCreateTrueColor (int sx, int sy)

// Check for OOM before doing a potentially large allocation.
auto allocsz = sizeof(gdImage)
+ sy * (sizeof(int *) + sizeof(unsigned char *))
+ sx * sy * (sizeof(int) + sizeof(unsigned char));
+ (sizeof(int *) + sizeof(unsigned char *)) * sy
+ (sizeof(int) + sizeof(unsigned char)) * sx * sy;
if (UNLIKELY(precheckOOM(allocsz))) {
// Don't throw here because GD might need to do its own cleanup.
return NULL;
Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/ext/ldap/ext_ldap.cpp
Expand Up @@ -2139,7 +2139,7 @@ String HHVM_FUNCTION(ldap_escape,

char hex[] = "0123456789abcdef";

String result(3 * value.size(), ReserveString);
String result(3UL * value.size(), ReserveString);
char *rdata = result.get()->mutableData(), *r = rdata;

for (int i = 0; i < value.size(); i++) {
Expand Down
4 changes: 2 additions & 2 deletions hphp/runtime/ext/xmlreader/ext_xmlreader.cpp
Expand Up @@ -598,11 +598,11 @@ Variant HHVM_METHOD(XMLReader, expand,
if (!basenode.isNull()) {
auto dombasenode = Native::data<DOMNode>(basenode.toObject());
doc = dombasenode->doc();
docp = doc->docp();
if (docp == nullptr) {
if (doc == nullptr || doc->docp() == nullptr) {
raise_warning("Invalid State Error");
return false;
}
docp = doc->docp();
}

if (data->m_ptr) {
Expand Down
59 changes: 46 additions & 13 deletions hphp/runtime/server/admin-request-handler.cpp
Expand Up @@ -76,6 +76,9 @@
#endif

#include <folly/Conv.h>
#include <folly/File.h>
#include <folly/FileUtil.h>
#include <folly/Optional.h>
#include <folly/Random.h>
#include <folly/portability/Unistd.h>

Expand Down Expand Up @@ -254,6 +257,34 @@ void AdminRequestHandler::teardownRequest(Transport* transport) noexcept {
WarnIfNotOK(transport);
}

namespace {

// When this struct is destroyed, it will close the file.
struct DumpFile {
std::string path;
folly::File file;
};

folly::Optional<DumpFile> dump_file(const char* name) {
auto const path = folly::sformat("{}/{}", RO::AdminDumpPath, name);

// mkdir -p the directory prefix of `path`
if (FileUtil::mkdir(path) != 0) return folly::none;

// If remove fails because of a permissions issue, then we won't be
// able to open the file for exclusive write below.
remove(path.c_str());

// Create the file, failing if it already exists. Doing so ensures
// that we have write access to the file and that no other user does.
auto const fd = open(path.c_str(), O_CREAT|O_EXCL|O_RDWR, 0666);
if (fd < 0) return folly::none;

return DumpFile{path, folly::File(fd, /*owns=*/true)};
}

}

void AdminRequestHandler::handleRequest(Transport *transport) {
transport->addHeader("Content-Type", "text/plain");
std::string cmd = transport->getCommand();
Expand Down Expand Up @@ -714,10 +745,12 @@ void AdminRequestHandler::handleRequest(Transport *transport) {
break;
}
if (strncmp(cmd.c_str(), "dump-static-strings", 19) == 0) {
auto filename = transport->getParam("file");
if (filename == "") filename = "/tmp/static_strings";
handleDumpStaticStringsRequest(cmd, filename);
transport->sendString("OK\n");
if (auto file = dump_file("static_strings")) {
handleDumpStaticStringsRequest(file->file);
transport->sendString(folly::sformat("dumped to {}\n", file->path));
} else {
transport->sendString("Unable to mkdir or file already exists.\n");
}
break;
}
if (strncmp(cmd.c_str(), "random-static-strings", 21) == 0) {
Expand Down Expand Up @@ -746,10 +779,12 @@ void AdminRequestHandler::handleRequest(Transport *transport) {
}

if (cmd == "dump-pcre-cache") {
auto filename = transport->getParam("file");
if (filename == "") filename = "/tmp/pcre_cache";
pcre_dump_cache(filename);
transport->sendString("OK\n");
if (auto file = dump_file("pcre_cache")) {
pcre_dump_cache(file->file);
transport->sendString(folly::sformat("dumped to {}\n", file->path));
} else {
transport->sendString("Unable to mkdir or file already exists.\n");
}
break;
}

Expand Down Expand Up @@ -1368,13 +1403,11 @@ std::string formatStaticString(StringData* str) {
"----\n{} bytes\n{}\n", str->size(), str->toCppString());
}

bool AdminRequestHandler::handleDumpStaticStringsRequest(
const std::string& /*cmd*/, const std::string& filename) {
bool AdminRequestHandler::handleDumpStaticStringsRequest(folly::File& file) {
auto const& list = lookupDefinedStaticStrings();
std::ofstream out(filename.c_str());
SCOPE_EXIT { out.close(); };
for (auto item : list) {
out << formatStaticString(item);
auto const line = formatStaticString(item);
folly::writeFull(file.fd(), line.data(), line.size());
if (RuntimeOption::EvalPerfDataMap) {
auto const len = std::min<size_t>(item->size(), 255);
std::string str(item->data(), len);
Expand Down
5 changes: 3 additions & 2 deletions hphp/runtime/server/admin-request-handler.h
Expand Up @@ -19,6 +19,8 @@
#include "hphp/runtime/server/access-log.h"
#include "hphp/runtime/server/server.h"

#include <folly/File.h>

namespace HPHP {
///////////////////////////////////////////////////////////////////////////////

Expand Down Expand Up @@ -72,8 +74,7 @@ struct AdminRequestHandler : RequestHandler {
Transport *transport);
bool handleStaticStringsRequest(const std::string &cmd,
Transport *transport);
bool handleDumpStaticStringsRequest(const std::string &cmd,
const std::string &filename);
bool handleDumpStaticStringsRequest(folly::File& file);
bool handleRandomStaticStringsRequest(const std::string &cmd,
Transport *transport);
bool handleVMRequest (const std::string &cmd, Transport *transport);
Expand Down
9 changes: 9 additions & 0 deletions hphp/test/slow/ext_ldap/t78806688.php
@@ -0,0 +1,9 @@
<?hh

<<__EntryPoint>>
function main(): void {
$multiplier = 1431655769;
$s = str_repeat("a", $multiplier);
ldap_escape($s);
echo "FAIL!\n";
}
1 change: 1 addition & 0 deletions hphp/test/slow/ext_ldap/t78806688.php.expectf
@@ -0,0 +1 @@
Fatal error: String length exceeded: 4294967307 > %d in %s/t78806688.php on line 7
11 changes: 11 additions & 0 deletions hphp/test/slow/ext_string/t76103217.php
@@ -0,0 +1,11 @@
<?hh

<<__EntryPoint>>
function main(): void {
$key = "\x26\xbd\xbd\xbd\xff\x60\xbf\xff\xff\x60";
$salt1 = "\x24\x32\x78\x24\x31\x30\x24\x24\x35\x24\xad\x20\x20\x26\xff\x60\xbf\xff\xff\x60\x24\x31\x78\xa8\xa8\xa0\x01\x01\x01\x01\x01\x01";
$salt2 = "\x24\x32\x78\x24\x31\x30\x24\x24\x35";

var_dump(base64_encode(crypt($key, $salt1)));
var_dump(base64_encode(crypt($key, $salt2)));
}
2 changes: 2 additions & 0 deletions hphp/test/slow/ext_string/t76103217.php.expect
@@ -0,0 +1,2 @@
string(4) "KjA="
string(12) "JDJ4JDEwJCQ1"
9 changes: 9 additions & 0 deletions hphp/test/slow/ext_xml/xmlreader_empty_expand.php
@@ -0,0 +1,9 @@
<?hh

<<__EntryPoint>>
function main(): void {
$a = new XMLReader();
$b = new DOMNode();
$a->expand($b);
$a->expand($a);
}
3 changes: 3 additions & 0 deletions hphp/test/slow/ext_xml/xmlreader_empty_expand.php.expectf
@@ -0,0 +1,3 @@
Warning: Invalid State Error in %s on line %d

Warning: Invalid State Error in %s on line %d

0 comments on commit abe0b29

Please sign in to comment.