Neels Janosch Hofmeyr <neels_at_elego.de> writes:
> And Here is an improved version *3* of the fix for issue 1796.
> A stupid debugging line had crept into the previous patch.
Committed in r31614, thanks! (Please see that rev for some tweaks I
made to the log message and the code.)
-Karl
> [[[
> Fix issue #1796: defective or malicious client can corrupt repository
> log messages.
> Also adding regression test for issue 1796.
>
> * subversion/include/private/svn_utf_private.h: Add this private header
> file.
> (svn_utf__is_valid): Move the function declaration here from
> libsvn_subr/utf_impl.h, because it is needed in libsvn_repos.
>
> * subversion/libsvn_subr/utf_impl.h: Include private/svn_utf_private.h.
> (svn_utf__is_valid): Move declaration away to svn_utf_private.h
> because this function is needed in libsvn_repos.
> (svn_utf__last_valid): Add comment to also see svn_utf__is_valid.
>
> * subversion/libsvn_repos/fs-wrap.c (validate_prop): Add two validations
> for SVN_PROP_REVISION_LOG's value. Validate UTF-8 encoding using
> svn_utf__is_valid, and validate consistent LF eol style by looking
> for and rejecting CR (\r) characters.
>
> * subversion/include/svn_repos.h (svn_repos_fs_change_node_prop):
> Change comment to describe the new property validation introduced
> in libsvn_repos/fs-wrap.c (validate_prop).
>
> * subversion/tests/libsvn_repos/repos-test.c
> (prop_validation): Add this regression test for issue 1796, which
> tries to commit two invalid log messages concerning UTF-8 and LF.
> (prop_validation_commit_with_revprop): Add this helper function for
> prop_validation, which runs a commit with a given revprop.
> (commit_callback_dummy): Add this svn_commit_callback2_t function to
> use it in prop_validation_commit_with_revprop. It does nothing.
>
> Patch by: neels
> Review by: danielsh
> ]]]
>
> --
> Neels Hofmeyr -- elego Software Solutions GmbH
> Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
> phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
> http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
> Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
>
>
>
>
>
>
>
> Index: subversion/include/private/svn_utf_private.h
> ===================================================================
> --- subversion/include/private/svn_utf_private.h (revision 0)
> +++ subversion/include/private/svn_utf_private.h (revision 0)
> @@ -0,0 +1,41 @@
> +/**
> + * @copyright
> + * ====================================================================
> + * Copyright (c) 2008 CollabNet. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + * @endcopyright
> + *
> + * @file svn_utf_private.h
> + * @brief UTF validation routines
> + */
> +
> +#ifndef SVN_UTF_PRIVATE_H
> +#define SVN_UTF_PRIVATE_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +
> +/* Return TRUE if the string SRC of length LEN is a valid UTF-8 encoding
> + * according to the rules laid down by the Unicode 4.0 standard, FALSE
> + * otherwise. This function is faster than svn_utf__last_valid.
> + */
> +svn_boolean_t svn_utf__is_valid(const char *src, apr_size_t len);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* SVN_UTF_PRIVATE_H */
> Index: subversion/libsvn_subr/utf_impl.h
> ===================================================================
> --- subversion/libsvn_subr/utf_impl.h (revision 31592)
> +++ subversion/libsvn_subr/utf_impl.h (working copy)
> @@ -24,6 +24,7 @@
>
> #include <apr_pools.h>
> #include "svn_types.h"
> +#include "private/svn_utf_private.h"
>
> #ifdef __cplusplus
> extern "C" {
> @@ -44,15 +45,10 @@
> * it will point to the start of the first invalid multi-byte character.
> * In either case all the characters between SRC and the return pointer are
> * valid UTF-8.
> + * See also svn_utf__is_valid.
> */
> const char *svn_utf__last_valid(const char *src, apr_size_t len);
>
> -/* Return TRUE if the string SRC of length LEN is a valid UTF-8 encoding
> - * according to the rules laid down by the Unicode 4.0 standard, FALSE
> - * otherwise. This function is faster than svn_utf__last_valid.
> - */
> -svn_boolean_t svn_utf__is_valid(const char *src, apr_size_t len);
> -
> /* As for svn_utf__is_valid but SRC is NULL terminated. */
> svn_boolean_t svn_utf__cstring_is_valid(const char *src);
>
> Index: subversion/libsvn_repos/fs-wrap.c
> ===================================================================
> --- subversion/libsvn_repos/fs-wrap.c (revision 31592)
> +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> @@ -28,6 +28,7 @@
> #include "svn_time.h"
> #include "repos.h"
> #include "svn_private_config.h"
> +#include "private/svn_utf_private.h"
>
>
> /*** Commit wrappers ***/
> @@ -169,6 +170,25 @@
> /* Validate "svn:" properties. */
> if (svn_prop_is_svn_prop(name) && value != NULL)
> {
> + /* Validate log message format to be UTF-8 with LF line endings. */
> + if (strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + {
> + /* Validate encoding of log message to be UTF-8. */
> + if (svn_utf__is_valid(value->data, value->len) == FALSE)
> + return svn_error_create
> + (SVN_ERR_BAD_PROPERTY_VALUE, NULL,
> + _("Cannot accept log message because it is not encoded in "
> + "UTF-8"));
> +
> + /* Disallow inconsistent line ending style, by simply looking for
> + * carriage return characters ('\r'). */
> + if (strchr(value->data, '\r') != NULL)
> + return svn_error_create
> + (SVN_ERR_BAD_PROPERTY_VALUE, NULL,
> + _("Cannot accept non-LF line endings "
> + "in log message"));
> + }
> +
> /* "svn:date" should be a valid date. */
> if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> {
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 31592)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -1805,8 +1805,9 @@
> * @a value and return SVN_ERR_BAD_PROPERTY_VALUE if it is invalid for the
> * property.
> *
> - * @note Currently, the only "svn:" property validated is @c
> - * SVN_PROP_REVISION_DATE. This may change in a future release.
> + * @note Currently, the only properties validated are the "svn:" properties
> + * @c SVN_PROP_REVISION_LOG and @c SVN_PROP_REVISION_DATE. This may change
> + * in future releases.
> */
> svn_error_t *
> svn_repos_fs_change_node_prop(svn_fs_root_t *root,
> Index: subversion/tests/libsvn_repos/repos-test.c
> ===================================================================
> --- subversion/tests/libsvn_repos/repos-test.c (revision 31592)
> +++ subversion/tests/libsvn_repos/repos-test.c (working copy)
> @@ -2187,6 +2187,164 @@
>
>
>
> +/* Test if prop values received by the server are validated.
> + * These tests "send" property values to the server and diagnose the
> + * behaviour.
> + */
> +
> +/* This is a svn_commit_callback2_t that does nothing.
> + */
> +static svn_error_t *commit_callback_dummy
> + (const svn_commit_info_t *commit_info, void *baton, apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
> +/* Helper function that makes an arbitrary change to a given repository
> + * REPOS and runs a commit with a specific revision property set to a
> + * certain value. The property name, type and value are given in PROP_KEY,
> + * PROP_KLEN and PROP_VAL, as in apr_hash_set(), using a const char* key.
> + *
> + * The FILENAME argument names a file in the test repository to add in
> + * this commit, e.g. "/A/should_fail_1".
> + *
> + * On success, the given file is added to the repository. So, using
> + * the same name multiple times on the same repository might fail. Thus,
> + * use different FILENAME arguments for every call to this function
> + * (e.g. "/A/f1", "/A/f2", "/A/f3" etc).
> + */
> +static svn_error_t *
> +prop_validation_commit_with_revprop(const char *filename,
> + const char *prop_key,
> + apr_ssize_t prop_klen,
> + const void *prop_val,
> + svn_repos_t *repos,
> + apr_pool_t *pool)
> +{
> + const svn_delta_editor_t *editor;
> + void *edit_baton;
> + void *root_baton;
> + void *file_baton;
> +
> + /* Prepare revision properties */
> + apr_hash_t *revprop_table = apr_hash_make(pool);
> +
> + /* Add the requested property */
> + apr_hash_set(revprop_table, prop_key, prop_klen, prop_val);
> +
> + /* Set usual author and log props, if not set already */
> + if (strcmp(prop_key, SVN_PROP_REVISION_AUTHOR) != 0)
> + {
> + apr_hash_set(revprop_table, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING,
> + svn_string_create("plato", pool));
> + }
> + else
> + if (strcmp(prop_key, SVN_PROP_REVISION_LOG) != 0)
> + {
> + apr_hash_set(revprop_table, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING,
> + svn_string_create("revision log", pool));
> + }
> +
> + /* Make an arbitrary change and commit using above values... */
> +
> + SVN_ERR(svn_repos_get_commit_editor5(&editor, &edit_baton, repos,
> + NULL, "file://test", "/",
> + revprop_table,
> + commit_callback_dummy, NULL,
> + NULL, NULL, pool));
> +
> + SVN_ERR(editor->open_root(edit_baton, 0, pool, &root_baton));
> +
> + SVN_ERR(editor->add_file(filename, root_baton, NULL,
> + SVN_INVALID_REVNUM, pool,
> + &file_baton));
> +
> + SVN_ERR(editor->close_file(file_baton, NULL, pool));
> +
> + SVN_ERR(editor->close_directory(root_baton, pool));
> +
> + SVN_ERR(editor->close_edit(edit_baton, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +/* Expect failure of invalid commit in these cases:
> + * - log message contains invalid UTF-8 octet (issue 1796)
> + * - log message contains invalid linefeed style (non-LF) (issue 1796)
> + */
> +static svn_error_t *
> +prop_validation(const char **msg,
> + svn_boolean_t msg_only,
> + svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err;
> + svn_repos_t *repos;
> + const char non_utf8_string[5] = { 'a', 0xff, 'b', '\n', 0 };
> + const char *non_lf_string = "a\r\nb\n\rc\rd\n";
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + *msg = "test if revprops are validated by repos";
> +
> + if (msg_only)
> + return SVN_NO_ERROR;
> +
> + /* Create a filesystem and repository. */
> + SVN_ERR(svn_test__create_repos(&repos, "test-repo-prop-validation",
> + opts, subpool));
> +
> +
> + /* Test an invalid commit log message: UTF-8 */
> + err = prop_validation_commit_with_revprop
> + ("/non_utf8_log_msg",
> + SVN_PROP_REVISION_LOG, APR_HASH_KEY_STRING,
> + svn_string_create(non_utf8_string, subpool),
> + repos, subpool);
> +
> + if (err == SVN_NO_ERROR)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Failed to reject a log with invalid "
> + "UTF-8");
> + else
> + if (err->apr_err != SVN_ERR_BAD_PROPERTY_VALUE)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Expected SVN_ERR_BAD_PROPERTY_VALUE for "
> + "a log with invalid UTF-8, "
> + "got another error.");
> + svn_error_clear(err);
> +
> +
> + /* Test an invalid commit log message: LF */
> + err = prop_validation_commit_with_revprop
> + ("/non_lf_log_msg",
> + SVN_PROP_REVISION_LOG, APR_HASH_KEY_STRING,
> + svn_string_create(non_lf_string, subpool),
> + repos, subpool);
> +
> + if (err == SVN_NO_ERROR)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Failed to reject a log with inconsistent "
> + "line ending style");
> + else
> + if (err->apr_err != SVN_ERR_BAD_PROPERTY_VALUE)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Expected SVN_ERR_BAD_PROPERTY_VALUE for "
> + "a log with inconsistent line ending style, "
> + "got another error.");
> + svn_error_clear(err);
> +
> +
> + /* Done. */
> + svn_pool_destroy(subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +
> /* The test table. */
>
> struct svn_test_descriptor_t test_funcs[] =
> @@ -2203,5 +2361,6 @@
> SVN_TEST_PASS(commit_continue_txn),
> SVN_TEST_PASS(node_location_segments),
> SVN_TEST_PASS(reporter_depth_exclude),
> + SVN_TEST_PASS(prop_validation),
> SVN_TEST_NULL
> };
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-06 20:43:41 CEST