Fix a low impact directory traversal bug

A common pattern for checking directories in ZNC is the following:

	sAbsolutePath = CDir::ChangeDir(sAllowedPath, sFile);
	if (sAbsolutePath.Left(sAllowedPath.length()) != sAllowedPath)
		Error;

But there is a problem: If sAllowedPath doesn't end with a slash, we are
vulnerable to an attack. If e.g. sAllowedPath = "/foo/bar", then
sFile = "../bartender" would result in sAbsolutePath = "/foo/bartender". Since
this path does begin with sAllowedPath, the code allowed it.

There shouldn't be any places where this can be exploited currently, but it is
still a security bug (path traversal).


git-svn-id: https://znc.svn.sourceforge.net/svnroot/znc/trunk@1569 726aef4b-f618-498e-8847-2d620e286838

ClientCommand.cpp

@@ -397,9 +397,9 @@ void CClient::UserCommand(CString& sLine) {
 			return;
 		}
 
-		sAbsolutePath = CDir::ChangeDir(m_pUser->GetDLPath(), sFile, CZNC::Get().GetHomePath());
+		sAbsolutePath = CDir::CheckPathPrefix(sAllowedPath, sFile);
 
-		if (sAbsolutePath.Left(sAllowedPath.length()) != sAllowedPath) {
+		if (sAbsolutePath.empty()) {
 			PutStatus("Illegal path.");
 			return;
 		}
@@ -415,9 +415,9 @@ void CClient::UserCommand(CString& sLine) {
 			return;
 		}
 
-		sAbsolutePath = CDir::ChangeDir(m_pUser->GetDLPath(), sFile, CZNC::Get().GetHomePath());
+		sAbsolutePath = CDir::CheckPathPrefix(sAllowedPath, sFile);
 
-		if (sAbsolutePath.Left(sAllowedPath.length()) != sAllowedPath) {
+		if (sAbsolutePath.empty()) {
 			PutStatus("Illegal path.");
 			return;
 		}

FileUtils.cpp

@@ -467,6 +467,15 @@ CString CDir::ChangeDir(const CString& sPath, const CString& sAdd, const CString
 	return (sRet.empty()) ? "/" : sRet;
 }
 
+CString CDir::CheckPathPrefix(const CString& sPath, const CString& sAdd, const CString& sHomeDir) {
+	CString sPrefix = sPath.Replace_n("//", "/").TrimRight_n("/") + "/";
+	CString sAbsolutePath = ChangeDir(sPrefix, sAdd, sHomeDir);
+
+	if (sAbsolutePath.Left(sPrefix.length()) != sPrefix)
+		return "";
+	return sAbsolutePath;
+}
+
 bool CDir::MakeDir(const CString& sPath, mode_t iMode) {
 	CString sDir;
 	VCString dirs;

FileUtils.h

@@ -263,6 +263,9 @@ public:
 	CFile::EFileAttr GetSortAttr() { return m_eSortAttr; }
 	bool IsDescending() { return m_bDesc; }
 
+	// Check if sPath + "/" + sAdd (~/ is handled) is an absolute path which
+	// resides under sPath. Returns absolute path on success, else "".
+	static CString CheckPathPrefix(const CString& sPath, const CString& sAdd, const CString& sHomeDir = "");
 	static CString ChangeDir(const CString& sPath, const CString& sAdd, const CString& sHomeDir = "");
 	static bool MakeDir(const CString& sPath, mode_t iMode = 0700);
 

HTTPSock.cpp

@@ -119,9 +119,9 @@ bool CHTTPSock::PrintFile(const CString& sFileName, CString sContentType) {
 	if (!m_sDocRoot.empty()) {
 		sFilePath.TrimLeft("/");
 
-		sFilePath = CDir::ChangeDir(m_sDocRoot, sFilePath, m_sDocRoot);
+		sFilePath = CDir::CheckPathPrefix(m_sDocRoot, sFilePath, m_sDocRoot);
 
-		if (sFilePath.Left(m_sDocRoot.size()) != m_sDocRoot) {
+		if (sFilePath.empty()) {
 			PrintErrorPage(403, "Forbidden", "You don't have permission to access that file on this server.");
 			DEBUG("THIS FILE:     [" << sFilePath << "] does not live in ...");
 			DEBUG("DOCUMENT ROOT: [" << m_sDocRoot << "]");

Template.cpp

@@ -106,6 +106,8 @@ CString CTemplate::ExpandFile(const CString& sFilename) {
 		CString sFilePath(CDir::ChangeDir(sRoot, sFile));
 
 		if (CFile::Exists(sFilePath)) {
+			// This only works if sRoot got a trailing slash! The
+			// code which adds paths makes sure this is true.
 			if (sRoot.empty() || sFilePath.Left(sRoot.length()) == sRoot) {
 				//DEBUG("\t\tFound  [" + sFilePath + "]\n");
 				return sFilePath;

modules/webadmin.cpp

@@ -254,12 +254,11 @@ CString CWebAdminSock::GetAvailSkinsDir() {
 CString CWebAdminSock::GetSkinDir() {
 	CString sAvailSkins = GetAvailSkinsDir();
 	CString sSkinDir = sAvailSkins + GetModule()->GetSkinName() + "/";
-	CString sDir = CDir::ChangeDir("./", sSkinDir, "/");
+	CString sDir = CDir::CheckPathPrefix("./", sSkinDir, "/");
 
-	// Via ChangeDir() we check if someone tries to use e.g. a skin name
+	// Via CheckPrefix() we check if someone tries to use e.g. a skin name
 	// with embed .. or such evilness.
-	if (sDir.Left(sAvailSkins.length()) == sAvailSkins
-			&& CFile::IsDir(sSkinDir)) {
+	if (!sDir.empty() && CFile::IsDir(sSkinDir)) {
 		return sSkinDir;
 	}