Skip to content

Commit

Permalink
Fix random file reading by scattered snippets
Browse files Browse the repository at this point in the history
Issue was that load_files_scattered option just concatenated prefix with
given name and then opened the file. So, if you provide something like
'/etc/password' as file, or '../../../etc/passwd' even with non-zero
prefix, it will unfortunately work. After this commit such behavior
possible ONLY if user explicitly set `snippets_file_prefix` to empty
string or '/'; another cases will not cause uncontroled reading.
That fixes #866
  • Loading branch information
klirichek committed Jul 2, 2019
1 parent 66b5761 commit 6e597ff
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1420,7 +1420,7 @@ snippets_file_prefix
~~~~~~~~~~~~~~~~~~~~

A prefix to prepend to the local file names when generating snippets.
Optional, default is empty.
Optional, default is current working folder.

This prefix can be used in distributed snippets generation along with
``load_files`` or ``load_files_scattered`` options.
Expand All @@ -1431,6 +1431,19 @@ prefix is set to “server1” and the request refers to “file23”,
quotes). So if you need it to be a path, you have to mention the
trailing slash.

After constructing final file path, daemon unwinds all relative dirs and
compares final result with the value of ``snippet_file_prefix``. If result
is not begin with the prefix, such file will be rejected with error message.

So, if you set it to '/mnt/data' and somebody calls snippet generation with file
'../../../etc/passwd', as the source, it will get error message
`File '/mnt/data/../../../etc/passwd' escapes '/mnt/data/' scope`
instead of content of the file.

Also, with non-set parameter and reading '/etc/passwd' it will actually read
/daemon/working/folder/etc/passwd since default for param is exactly daemon's
working folder.

Note also that this is a local option, it does not affect the agents
anyhow. So you can safely set a prefix on a master server. The requests
routed to the agents will not be affected by the master's setting. They
Expand All @@ -1448,6 +1461,10 @@ Example:
snippets_file_prefix = /mnt/common/server1/
.. warning::
If you still want to access files from the FS root, you have to explicitly set ``snippets_file_prefix``
to empty value (by `snippets_file_prefix=` line), or to root (by `snippets_file_prefix=/`).

.. _sphinxql_state:

sphinxql_state
Expand Down
8 changes: 8 additions & 0 deletions src/searchd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9996,6 +9996,12 @@ bool MakeSnippets ( CSphString sIndex, CSphVector<ExcerptQueryChained_t> & dQuer
{
CSphString sFilename, sStatError;
sFilename.SetSprintf ( "%s%s", g_sSnippetsFilePrefix.cstr(), dQuery.m_sSource.scstr() );
if ( !TestEscaping ( g_sSnippetsFilePrefix, sFilename ))
{
sError.SetSprintf( "File '%s' escapes '%s' scope",
sFilename.scstr(), g_sSnippetsFilePrefix.scstr());
return false;
}
auto iFileSize = sphGetFileSize (sFilename, &sStatError);
if ( iFileSize<0 )
{
Expand Down Expand Up @@ -24225,6 +24231,8 @@ int WINAPI ServiceMain ( int argc, char **argv ) REQUIRES (!MainThread)

if ( hSearchd.Exists ( "snippets_file_prefix" ) )
g_sSnippetsFilePrefix = hSearchd["snippets_file_prefix"].cstr();
else
g_sSnippetsFilePrefix.SetSprintf("%s/", sphGetCwd().scstr());

const char* sLogFormat = hSearchd.GetStr ( "query_log_format", "plain" );
if ( !strcmp ( sLogFormat, "sphinxql" ) )
Expand Down
8 changes: 8 additions & 0 deletions src/sphinx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,11 @@

#if USE_WINDOWS
// workaround Windows quirks
#include <direct.h>
#define popen _popen
#define pclose _pclose
#define snprintf _snprintf
#define getcwd _getcwd

#define stat _stat64
#define fstat _fstat64
Expand Down Expand Up @@ -11057,6 +11059,12 @@ CSphString sphNormalizePath( const CSphString& sOrigPath )
return sResult.cstr();
}

CSphString sphGetCwd()
{
CSphVector<char> sBuf (65536);
return getcwd( sBuf.begin(), sBuf.GetLength());
}

class DeleteOnFail_c : public ISphNoncopyable
{
public:
Expand Down
14 changes: 14 additions & 0 deletions src/sphinxexcerpt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3784,6 +3784,11 @@ void SnippetContext_t::BuildExcerpt ( ExcerptQuery_t & tOptions, const CSphIndex
CSphAutofile tFile;
CSphString sFilename;
sFilename.SetSprintf ( "%s%s", g_sSnippetsFilePrefix.cstr(), tOptions.m_sSource.scstr() );
if ( !TestEscaping( g_sSnippetsFilePrefix, sFilename ))
{
sError.SetSprintf( "File '%s' escapes '%s' scope", sFilename.scstr(), g_sSnippetsFilePrefix.scstr());
return;
}
if ( !sFilename.IsEmpty () && tFile.Open ( sFilename.cstr(), SPH_O_READ, sError )<0 )
return;
else if ( tOptions.m_sSource.IsEmpty() )
Expand Down Expand Up @@ -3973,6 +3978,15 @@ bool SnippetContext_t::Setup ( const CSphIndex * pIndex, const ExcerptQuery_t &t
return SetupStripperSPZ ( pIndex->GetSettings (), tSettings, bSetupSPZ, m_tStripper, m_pTokenizer, sError );
}

// check whether filepath from sPath does not escape area of sPrefix
bool TestEscaping( const CSphString& sPrefix, const CSphString& sPath )
{
if ( sPrefix.IsEmpty() || sPrefix==sPath )
return true;
auto sNormalized = sphNormalizePath( sPath );
return sPrefix==sNormalized.SubString( 0, sPrefix.Length());
}

//
// $Id$
//
3 changes: 3 additions & 0 deletions src/sphinxexcerpt.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,8 @@ class SnippetContext_t : ISphNoncopyable
void BuildExcerpt ( ExcerptQuery_t &tOptions, const CSphIndex * pIndex ) const;
};

// helper whether filepath from sPath does not escape area of sPrefix
bool TestEscaping( const CSphString& sPrefix, const CSphString& sPath );

extern CSphString g_sSnippetsFilePrefix;
#endif // _sphinxexcerpt_
2 changes: 2 additions & 0 deletions src/sphinxstd.h
Original file line number Diff line number Diff line change
Expand Up @@ -3081,6 +3081,8 @@ bool sphTruncate ( int iFD );

// unwind different tricks like "../../../etc/passwd"
CSphString sphNormalizePath ( const CSphString& sOrigPath );
CSphString sphGetCwd();


/// buffer trait that neither own buffer nor clean-up it on destroy
template < typename T >
Expand Down
7 changes: 4 additions & 3 deletions test/test_130/test.xml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
searchd
{
<searchd_settings/>
snippets_file_prefix=<this_test/>/
}

source test
Expand All @@ -31,15 +32,15 @@ index test
global $this_test;
$results = array();
$docs = array( "$this_test/load_file.txt" );
$docs = array( "load_file.txt" );
$opts = array( 'load_files'=>true, 'limit'=>0 );
$results[] = $client->BuildExcerpts($docs, 'test', 'end point', $opts );
$results[] = $client->BuildExcerpts($docs, 'test', 'not_found', $opts );
$results[] = $client->BuildExcerpts(array( "$this_test/empty.txt" ), 'test', 'end point', $opts );
$results[] = $client->BuildExcerpts(array( "empty.txt" ), 'test', 'end point', $opts );
$results[] = $client->BuildExcerpts(array( '' ), 'test', 'not_found', $opts );
$results[] = $client->GetLastError();
$results[] = $client->BuildExcerpts ( array ( "$this_test/512k.xml" ), 'test', 'it builds', array ( "limit" => 100, "load_files" => true ) );
$results[] = $client->BuildExcerpts ( array ( "512k.xml" ), 'test', 'it builds', array ( "limit" => 100, "load_files" => true ) );
]]></custom_test>

Expand Down

0 comments on commit 6e597ff

Please sign in to comment.