https://bugzilla.redhat.com/show_bug.cgi?id=17330884 https://subversion.apache.org/security/CVE-2018-11782-advisory.txt Fixes for CVE-2018-11782, svnserve get-deleted-rev assertion failure. The svn protocol prototype for get-deleted-rev does not allow for a reply of SVN_INVALID_REVNUM directly. A query having such an answer previously caused the server to raise an assertion failure which could crash the whole process or a thread or child process of it, depending on the build configuration of the server. To work around the problem without changing the protocol, we re-purpose the obsolete error code 'SVN_ERR_ENTRY_MISSING_REVISION' to communicate this 'not deleted' reply to the client. - With a new client against a new server, such queries are now handled correctly. - With an old client against a new server, the client will report a more informative error message, and the server will not crash. - With a new client against an old server, the behaviour is the same as with an old client against an old server. In addition, this fixes a similar problem whereby any regular error response to a 'get-deleted-rev' query resulted in the server closing the connection, process and/or thread (again depending on the build configuration). Now such errors are correctly passed back to the client. * subversion/libsvn_ra_svn/client.c (ra_svn_get_deleted_rev): Detect error SVN_ERR_ENTRY_MISSING_REVISION and convert it to a response of SVN_INVALID_REVNUM. * subversion/svnserve/serve.c (get_deleted_rev): Respond with error SVN_ERR_ENTRY_MISSING_REVISION instead of an assertion failure if the answer is SVN_INVALID_REVNUM. If svn_repos_deleted_rev() returns an error, pass that error back to the client. * subversion/tests/libsvn_ra/ra-test.c (commit_two_changes): New. (test_get_deleted_rev_no_delete, test_get_deleted_rev_errors): New tests. (test_funcs): Run them. --This line, and those below, will be ignored-- Index: subversion/libsvn_ra_svn/client.c =================================================================== --- subversion-1.10.2/subversion/libsvn_ra_svn/client.c.cve11782 +++ subversion-1.10.2/subversion/libsvn_ra_svn/client.c @@ -3105,6 +3105,7 @@ { svn_ra_svn__session_baton_t *sess_baton = session->priv; svn_ra_svn_conn_t *conn = sess_baton->conn; + svn_error_t *err; path = reparent_path(session, path, pool); @@ -3116,8 +3117,20 @@ SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess_baton, pool), N_("'get-deleted-rev' not implemented"))); - return svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r", - revision_deleted)); + err = svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r", + revision_deleted)); + /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly. + Instead, a new enough server returns SVN_ERR_ENTRY_MISSING_REVISION to + indicate the answer to the query is SVN_INVALID_REVNUM. (An older server + closes the connection and returns SVN_ERR_RA_SVN_CONNECTION_CLOSED.) */ + if (err && err->apr_err == SVN_ERR_ENTRY_MISSING_REVISION) + { + *revision_deleted = SVN_INVALID_REVNUM; + svn_error_clear(err); + } + else + SVN_ERR(err); + return SVN_NO_ERROR; } static svn_error_t * --- subversion-1.10.2/subversion/svnserve/serve.c.cve11782 +++ subversion-1.10.2/subversion/svnserve/serve.c @@ -3505,8 +3505,21 @@ svn_relpath_canonicalize(path, pool), pool); SVN_ERR(log_command(b, conn, pool, "get-deleted-rev")); SVN_ERR(trivial_auth_request(conn, pool, b)); - SVN_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision, - end_revision, &revision_deleted, pool)); + SVN_CMD_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision, + end_revision, &revision_deleted, pool)); + + /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly. + Instead, return SVN_ERR_ENTRY_MISSING_REVISION. A new enough client + knows that this means the answer to the query is SVN_INVALID_REVNUM. + (An older client reports this as an error.) */ + if (revision_deleted == SVN_INVALID_REVNUM) + SVN_CMD_ERR(svn_error_createf(SVN_ERR_ENTRY_MISSING_REVISION, NULL, + "svn protocol command 'get-deleted-rev': " + "path '%s' was not deleted in r%ld-%ld; " + "NOTE: newer clients handle this case " + "and do not report it as an error", + full_path, peg_revision, end_revision)); + SVN_ERR(svn_ra_svn__write_cmd_response(conn, pool, "r", revision_deleted)); return SVN_NO_ERROR; } --- subversion-1.10.2/subversion/tests/libsvn_ra/ra-test.c.cve11782 +++ subversion-1.10.2/subversion/tests/libsvn_ra/ra-test.c @@ -94,6 +94,41 @@ return SVN_NO_ERROR; } +/* Commit two revisions: add 'B', then delete 'A' */ +static svn_error_t * +commit_two_changes(svn_ra_session_t *session, + apr_pool_t *pool) +{ + apr_hash_t *revprop_table = apr_hash_make(pool); + const svn_delta_editor_t *editor; + void *edit_baton; + void *root_baton, *dir_baton; + + /* mkdir B */ + SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton, + revprop_table, + NULL, NULL, NULL, TRUE, pool)); + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, + pool, &root_baton)); + SVN_ERR(editor->add_directory("B", root_baton, NULL, SVN_INVALID_REVNUM, + pool, &dir_baton)); + SVN_ERR(editor->close_directory(dir_baton, pool)); + SVN_ERR(editor->close_directory(root_baton, pool)); + SVN_ERR(editor->close_edit(edit_baton, pool)); + + /* delete A */ + SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton, + revprop_table, + NULL, NULL, NULL, TRUE, pool)); + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, + pool, &root_baton)); + SVN_ERR(editor->delete_entry("A", SVN_INVALID_REVNUM, root_baton, pool)); + SVN_ERR(editor->close_directory(root_baton, pool)); + SVN_ERR(editor->close_edit(edit_baton, pool)); + + return SVN_NO_ERROR; +} + static svn_error_t * commit_tree(svn_ra_session_t *session, apr_pool_t *pool) @@ -1784,6 +1819,56 @@ return SVN_NO_ERROR; } +/* Cases of 'get-deleted-rev' that should return SVN_INVALID_REVNUM. */ +static svn_error_t * +test_get_deleted_rev_no_delete(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_ra_session_t *ra_session; + svn_revnum_t revision_deleted; + + SVN_ERR(make_and_open_repos(&ra_session, + "test-repo-get-deleted-rev-no-delete", opts, + pool)); + SVN_ERR(commit_changes(ra_session, pool)); + SVN_ERR(commit_two_changes(ra_session, pool)); + + /* expect 'no deletion' in the range up to r2, when it is deleted in r3 */ + /* This was failing over RA-SVN where the 'get-deleted-rev' wire command's + prototype cannot directly represent that result. A new enough client and + server collaborate on a work-around implemented using an error code. */ + SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 2, + &revision_deleted, pool)); + SVN_TEST_INT_ASSERT(revision_deleted, SVN_INVALID_REVNUM); + + /* this connection should still be open: a simple case should still work */ + SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 3, + &revision_deleted, pool)); + SVN_TEST_INT_ASSERT(revision_deleted, 3); + + return SVN_NO_ERROR; +} + +/* Cases of 'get-deleted-rev' that should return an error. */ +static svn_error_t * +test_get_deleted_rev_errors(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_ra_session_t *ra_session; + svn_revnum_t revision_deleted; + + SVN_ERR(make_and_open_repos(&ra_session, + "test-repo-get-deleted-rev-errors", opts, pool)); + SVN_ERR(commit_changes(ra_session, pool)); + + /* expect an error when searching up to r3, when repository head is r1 */ + SVN_TEST_ASSERT_ERROR(svn_ra_get_deleted_rev(ra_session, "A", 1, 3, + &revision_deleted, pool), + SVN_ERR_FS_NO_SUCH_REVISION); + + return SVN_NO_ERROR; +} + /* The test table. */ @@ -1820,6 +1905,10 @@ "check how last change applies to empty commit"), SVN_TEST_OPTS_PASS(commit_locked_file, "check commit editor for a locked file"), + SVN_TEST_OPTS_PASS(test_get_deleted_rev_no_delete, + "test get-deleted-rev no delete"), + SVN_TEST_OPTS_PASS(test_get_deleted_rev_errors, + "test get-deleted-rev errors"), SVN_TEST_NULL };