| Author: Ben Hutchings <ben@decadent.org.uk> |
| Date: Tue, 19 May 2015 02:38:40 +0100 |
| Description: Delay creation of symlinks to prevent arbitrary file writes (CVE-2015-1038) |
| Bug: http://sourceforge.net/p/p7zip/bugs/147/ |
| Bug-Debian: https://bugs.debian.org/774660 |
| |
| Alexander Cherepanov discovered that 7zip is susceptible to a |
| directory traversal vulnerability. While extracting an archive, it |
| will extract symlinks and then follow them if they are referenced in |
| further entries. This can be exploited by a rogue archive to write |
| files outside the current directory. |
| |
| We have to create placeholder files (which we already do) and delay |
| creating symlinks until the end of extraction. |
| |
| Due to the possibility of anti-items (deletions) in the archive, it is |
| possible for placeholders to be deleted and replaced before we create |
| the symlinks. It's not clear that this can be used for mischief, but |
| GNU tar guards against similar problems by checking that the placeholder |
| still exists and is the same inode. XXX It also checks 'birth time' but |
| this isn't portable. We can probably get away with comparing ctime |
| since we don't support hard links. |
| |
| --- a/CPP/7zip/UI/Agent/Agent.cpp |
| +++ b/CPP/7zip/UI/Agent/Agent.cpp |
| @@ -424,6 +424,8 @@ STDMETHODIMP CAgentFolder::Extract(const |
| CMyComPtr<IArchiveExtractCallback> extractCallback = extractCallbackSpec; |
| UStringVector pathParts; |
| CProxyFolder *currentProxyFolder = _proxyFolderItem; |
| + HRESULT res; |
| + |
| while (currentProxyFolder->Parent) |
| { |
| pathParts.Insert(0, currentProxyFolder->Name); |
| @@ -445,8 +447,11 @@ STDMETHODIMP CAgentFolder::Extract(const |
| (UInt64)(Int64)-1); |
| CUIntVector realIndices; |
| GetRealIndices(indices, numItems, realIndices); |
| - return _agentSpec->GetArchive()->Extract(&realIndices.Front(), |
| + res = _agentSpec->GetArchive()->Extract(&realIndices.Front(), |
| realIndices.Size(), testMode, extractCallback); |
| + if (res == S_OK && !extractCallbackSpec->CreateSymLinks()) |
| + res = E_FAIL; |
| + return res; |
| COM_TRY_END |
| } |
| |
| --- a/CPP/7zip/UI/Agent/ArchiveFolder.cpp |
| +++ b/CPP/7zip/UI/Agent/ArchiveFolder.cpp |
| @@ -20,6 +20,8 @@ STDMETHODIMP CAgentFolder::CopyTo(const |
| CMyComPtr<IArchiveExtractCallback> extractCallback = extractCallbackSpec; |
| UStringVector pathParts; |
| CProxyFolder *currentProxyFolder = _proxyFolderItem; |
| + HRESULT res; |
| + |
| while (currentProxyFolder->Parent) |
| { |
| pathParts.Insert(0, currentProxyFolder->Name); |
| @@ -46,8 +48,11 @@ STDMETHODIMP CAgentFolder::CopyTo(const |
| (UInt64)(Int64)-1); |
| CUIntVector realIndices; |
| GetRealIndices(indices, numItems, realIndices); |
| - return _agentSpec->GetArchive()->Extract(&realIndices.Front(), |
| + res = _agentSpec->GetArchive()->Extract(&realIndices.Front(), |
| realIndices.Size(), BoolToInt(false), extractCallback); |
| + if (res == S_OK && !extractCallbackSpec->CreateSymLinks()) |
| + res = E_FAIL; |
| + return res; |
| COM_TRY_END |
| } |
| |
| --- a/CPP/7zip/UI/Client7z/Client7z.cpp |
| +++ b/CPP/7zip/UI/Client7z/Client7z.cpp |
| @@ -197,8 +197,11 @@ private: |
| COutFileStream *_outFileStreamSpec; |
| CMyComPtr<ISequentialOutStream> _outFileStream; |
| |
| + CObjectVector<NWindows::NFile::NDirectory::CDelayedSymLink> _delayedSymLinks; |
| + |
| public: |
| void Init(IInArchive *archiveHandler, const UString &directoryPath); |
| + bool CreateSymLinks(); |
| |
| UInt64 NumErrors; |
| bool PasswordIsDefined; |
| @@ -392,11 +395,22 @@ STDMETHODIMP CArchiveExtractCallback::Se |
| } |
| _outFileStream.Release(); |
| if (_extractMode && _processedFileInfo.AttribDefined) |
| - NFile::NDirectory::MySetFileAttributes(_diskFilePath, _processedFileInfo.Attrib); |
| + NFile::NDirectory::MySetFileAttributes(_diskFilePath, _processedFileInfo.Attrib, &_delayedSymLinks); |
| PrintNewLine(); |
| return S_OK; |
| } |
| |
| +bool CArchiveExtractCallback::CreateSymLinks() |
| +{ |
| + bool success = true; |
| + |
| + for (int i = 0; i != _delayedSymLinks.Size(); ++i) |
| + success &= _delayedSymLinks[i].Create(); |
| + |
| + _delayedSymLinks.Clear(); |
| + |
| + return success; |
| +} |
| |
| STDMETHODIMP CArchiveExtractCallback::CryptoGetTextPassword(BSTR *password) |
| { |
| --- a/CPP/7zip/UI/Common/ArchiveExtractCallback.cpp |
| +++ b/CPP/7zip/UI/Common/ArchiveExtractCallback.cpp |
| @@ -453,12 +453,24 @@ STDMETHODIMP CArchiveExtractCallback::Se |
| NumFiles++; |
| |
| if (_extractMode && _fi.AttribDefined) |
| - NFile::NDirectory::MySetFileAttributes(_diskFilePath, _fi.Attrib); |
| + NFile::NDirectory::MySetFileAttributes(_diskFilePath, _fi.Attrib, &_delayedSymLinks); |
| RINOK(_extractCallback2->SetOperationResult(operationResult, _encrypted)); |
| return S_OK; |
| COM_TRY_END |
| } |
| |
| +bool CArchiveExtractCallback::CreateSymLinks() |
| +{ |
| + bool success = true; |
| + |
| + for (int i = 0; i != _delayedSymLinks.Size(); ++i) |
| + success &= _delayedSymLinks[i].Create(); |
| + |
| + _delayedSymLinks.Clear(); |
| + |
| + return success; |
| +} |
| + |
| /* |
| STDMETHODIMP CArchiveExtractCallback::GetInStream( |
| const wchar_t *name, ISequentialInStream **inStream) |
| --- a/CPP/7zip/UI/Common/ArchiveExtractCallback.h |
| +++ b/CPP/7zip/UI/Common/ArchiveExtractCallback.h |
| @@ -6,6 +6,8 @@ |
| #include "Common/MyCom.h" |
| #include "Common/Wildcard.h" |
| |
| +#include "Windows/FileDir.h" |
| + |
| #include "../../IPassword.h" |
| |
| #include "../../Common/FileStreams.h" |
| @@ -83,6 +85,8 @@ class CArchiveExtractCallback: |
| UInt64 _packTotal; |
| UInt64 _unpTotal; |
| |
| + CObjectVector<NWindows::NFile::NDirectory::CDelayedSymLink> _delayedSymLinks; |
| + |
| void CreateComplexDirectory(const UStringVector &dirPathParts, UString &fullPath); |
| HRESULT GetTime(int index, PROPID propID, FILETIME &filetime, bool &filetimeIsDefined); |
| HRESULT GetUnpackSize(); |
| @@ -138,6 +142,7 @@ public: |
| const UStringVector &removePathParts, |
| UInt64 packSize); |
| |
| + bool CreateSymLinks(); |
| }; |
| |
| #endif |
| --- a/CPP/7zip/UI/Common/Extract.cpp |
| +++ b/CPP/7zip/UI/Common/Extract.cpp |
| @@ -96,6 +96,9 @@ static HRESULT DecompressArchive( |
| else |
| result = archive->Extract(&realIndices.Front(), realIndices.Size(), testMode, extractCallbackSpec); |
| |
| + if (result == S_OK && !extractCallbackSpec->CreateSymLinks()) |
| + result = E_FAIL; |
| + |
| return callback->ExtractResult(result); |
| } |
| |
| --- a/CPP/Windows/FileDir.cpp |
| +++ b/CPP/Windows/FileDir.cpp |
| @@ -453,9 +453,10 @@ bool SetDirTime(LPCWSTR fileName, const |
| } |
| |
| #ifndef _UNICODE |
| -bool MySetFileAttributes(LPCWSTR fileName, DWORD fileAttributes) |
| +bool MySetFileAttributes(LPCWSTR fileName, DWORD fileAttributes, |
| + CObjectVector<CDelayedSymLink> *delayedSymLinks) |
| { |
| - return MySetFileAttributes(UnicodeStringToMultiByte(fileName, CP_ACP), fileAttributes); |
| + return MySetFileAttributes(UnicodeStringToMultiByte(fileName, CP_ACP), fileAttributes, delayedSymLinks); |
| } |
| |
| bool MyRemoveDirectory(LPCWSTR pathName) |
| @@ -488,7 +489,8 @@ static int convert_to_symlink(const char |
| return -1; |
| } |
| |
| -bool MySetFileAttributes(LPCTSTR fileName, DWORD fileAttributes) |
| +bool MySetFileAttributes(LPCTSTR fileName, DWORD fileAttributes, |
| + CObjectVector<CDelayedSymLink> *delayedSymLinks) |
| { |
| if (!fileName) { |
| SetLastError(ERROR_PATH_NOT_FOUND); |
| @@ -520,7 +522,9 @@ bool MySetFileAttributes(LPCTSTR fileNam |
| stat_info.st_mode = fileAttributes >> 16; |
| #ifdef ENV_HAVE_LSTAT |
| if (S_ISLNK(stat_info.st_mode)) { |
| - if ( convert_to_symlink(name) != 0) { |
| + if (delayedSymLinks) |
| + delayedSymLinks->Add(CDelayedSymLink(name)); |
| + else if ( convert_to_symlink(name) != 0) { |
| TRACEN((printf("MySetFileAttributes(%s,%d) : false-3\n",name,fileAttributes))) |
| return false; |
| } |
| @@ -924,4 +928,41 @@ bool CTempDirectory::Create(LPCTSTR pref |
| } |
| |
| |
| +#ifdef ENV_UNIX |
| + |
| +CDelayedSymLink::CDelayedSymLink(LPCSTR source) |
| + : _source(source) |
| +{ |
| + struct stat st; |
| + |
| + if (lstat(_source, &st) == 0) { |
| + _dev = st.st_dev; |
| + _ino = st.st_ino; |
| + } else { |
| + _dev = 0; |
| + } |
| +} |
| + |
| +bool CDelayedSymLink::Create() |
| +{ |
| + struct stat st; |
| + |
| + if (_dev == 0) { |
| + errno = EPERM; |
| + return false; |
| + } |
| + if (lstat(_source, &st) != 0) |
| + return false; |
| + if (_dev != st.st_dev || _ino != st.st_ino) { |
| + // Placeholder file has been overwritten or moved by another |
| + // symbolic link creation |
| + errno = EPERM; |
| + return false; |
| + } |
| + |
| + return convert_to_symlink(_source) == 0; |
| +} |
| + |
| +#endif // ENV_UNIX |
| + |
| }}} |
| --- a/CPP/Windows/FileDir.h |
| +++ b/CPP/Windows/FileDir.h |
| @@ -4,6 +4,7 @@ |
| #define __WINDOWS_FILEDIR_H |
| |
| #include "../Common/MyString.h" |
| +#include "../Common/MyVector.h" |
| #include "Defs.h" |
| |
| /* GetFullPathName for 7zAES.cpp */ |
| @@ -13,11 +14,15 @@ namespace NWindows { |
| namespace NFile { |
| namespace NDirectory { |
| |
| +class CDelayedSymLink; |
| + |
| bool SetDirTime(LPCWSTR fileName, const FILETIME *creationTime, const FILETIME *lastAccessTime, const FILETIME *lastWriteTime); |
| |
| -bool MySetFileAttributes(LPCTSTR fileName, DWORD fileAttributes); |
| +bool MySetFileAttributes(LPCTSTR fileName, DWORD fileAttributes, |
| + CObjectVector<CDelayedSymLink> *delayedSymLinks = 0); |
| #ifndef _UNICODE |
| -bool MySetFileAttributes(LPCWSTR fileName, DWORD fileAttributes); |
| +bool MySetFileAttributes(LPCWSTR fileName, DWORD fileAttributes, |
| + CObjectVector<CDelayedSymLink> *delayedSymLinks = 0); |
| #endif |
| |
| bool MyMoveFile(LPCTSTR existFileName, LPCTSTR newFileName); |
| @@ -80,6 +85,31 @@ public: |
| bool Remove(); |
| }; |
| |
| +// Symbolic links must be created last so that they can't be used to |
| +// create or overwrite files above the extraction directory. |
| +class CDelayedSymLink |
| +{ |
| +#ifdef ENV_UNIX |
| + // Where the symlink should be created. The target is specified in |
| + // the placeholder file. |
| + AString _source; |
| + |
| + // Device and inode of the placeholder file. Before creating the |
| + // symlink, we must check that these haven't been changed by creation |
| + // of another symlink. |
| + dev_t _dev; |
| + ino_t _ino; |
| + |
| +public: |
| + explicit CDelayedSymLink(LPCSTR source); |
| + bool Create(); |
| +#else // !ENV_UNIX |
| +public: |
| + CDelayedSymLink(LPCSTR source) {} |
| + bool Create() { return true; } |
| +#endif // ENV_UNIX |
| +}; |
| + |
| #ifdef _UNICODE |
| typedef CTempFile CTempFileW; |
| #endif |