This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7815

sha1dc: initial implementation of Python extension
ClosedPublic

Authored by durin42 on Jan 8 2020, 4:27 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGbde1cd4c99d9: sha1dc: initial implementation of Python extension
Summary

A future change will use this when available to avoid sha1 collision
issues until we can get moved to something else.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

durin42 created this revision.Jan 8 2020, 4:27 PM
durin42 updated this revision to Diff 19113.Jan 8 2020, 4:31 PM
spectral added a subscriber: spectral.Jan 8 2020, 4:51 PM
spectral added inline comments.
mercurial/thirdparty/sha1dc/cext.c
25

Nit: "clang-format on" (typo: s/oon/on/)

105

I think you need a {NULL} entry here, or else it's going to walk off the end of this array when creating the type.

149

You may also need a NULL entry here, haven't tracked down where this is used.

153

why do this here instead of above?

durin42 edited the summary of this revision. (Show Details)Jan 8 2020, 5:38 PM
durin42 updated this revision to Diff 19115.
durin42 updated this revision to Diff 19116.
durin42 marked 4 inline comments as done.Jan 8 2020, 5:41 PM
durin42 added inline comments.
mercurial/thirdparty/sha1dc/cext.c
25

Good catch!

105

Sigh, yep.

153

Not sure. This is just how I've always seen it done...

durin42 marked 3 inline comments as done.Jan 8 2020, 5:44 PM
durin42 retitled this revision from sha1dc: initial implementation of Python extension (NOT READY) to sha1dc: initial implementation of Python extension.Jan 8 2020, 8:30 PM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 19119.
indygreg added a subscriber: indygreg.Jan 8 2020, 11:29 PM

This seems mostly good to me.

mercurial/thirdparty/sha1dc/cext.c
69

I'm not super keen on overloading OverflowError here. Or is this how hashlib works in the Python standard library?

93

I didn't verify this is correct because I didn't want to think too hard. Test coverage for parity with hashlib would be appreciated.

108

Should this be a memcpy or some such? Or is this opaque type safe to copy by value? Test coverage for this demonstrating that a seeded hasher which is copied can properly diverge would be appreciated.

173

Nit: please always use braces.

durin42 marked 3 inline comments as done.Jan 13 2020, 4:55 PM

Good call on tests, I had the return type of hexdigest() wrong. Please do let me know if you think of any missing cases here.

mercurial/thirdparty/sha1dc/cext.c
69

This is new functionality compared to hashlib (this is the "zomg someone is breaking ur sha1" error case), so there's not anything comparable. I didn't really want to go to the effort of defining our own custom exception type for a case we should never see in the wild.

108

I verified by inspection (when writing this) that copying by value is okay here (no pointers, etc), and we now have test coverage that will catch any regression here.

durin42 marked an inline comment as done.Jan 13 2020, 5:48 PM
durin42 added a child revision: D7848: hashutil: new package for hashing-related features.
durin42 updated this revision to Diff 19188.
indygreg accepted this revision.Jan 13 2020, 11:30 PM
This revision is now accepted and ready to land.Jan 13 2020, 11:30 PM
durin42 added a commit: rHGbde1cd4c99d9: sha1dc: initial implementation of Python extension.Jan 13 2020, 11:39 PM
Closed by commit rHGbde1cd4c99d9: sha1dc: initial implementation of Python extension (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
AMmercurial/thirdparty/sha1dc/cext.c (178 lines)
Msetup.py (8 lines)

Diff 19112

mercurial/thirdparty/sha1dc/cext.c

  • This file was added.
#include <Python.h>
#include "lib/sha1.h"
#if PY_MAJOR_VERSION >= 3
#define IS_PY3K
#endif
/* helper to switch things like string literal depending on Python version */
#ifdef IS_PY3K
#define PY23(py2, py3) py3
#else
#define PY23(py2, py3) py2
#endif
static char sha1collisiondetection_doc[] =
"Efficient detection of SHA1 collision constructs.";
/* clang-format off */
typedef struct {
PyObject_HEAD
SHA1_CTX ctx;
} pysha1ctx;
/* clang-format oon */
spectralUnsubmitted
Done

Nit: "clang-format on" (typo: s/oon/on/)

spectral: Nit: "clang-format on" (typo: s/oon/on/)
durin42AuthorUnsubmitted
Done

Good catch!

durin42: Good catch!
static int pysha1ctx_init(pysha1ctx *self, PyObject *args)
{
const char* data = NULL;
Py_ssize_t len;
SHA1DCInit(&(self->ctx));
/* We don't want "safe" sha1s, wherein sha1dc can give you a
different hash for something that's trying to give you a
collision. We just want to detect collisions.
*/
SHA1DCSetSafeHash(&(self->ctx), 0);
if (!PyArg_ParseTuple(args, PY23("|s#", "|y#"), &data, &len)) {
return -1;
}
if (data) {
SHA1DCUpdate(&(self->ctx), data, len);
}
return 0;
}
static void pysha1ctx_dealloc(pysha1ctx *self) {
PyObject_Del(self);
}
static PyObject *pysha1ctx_update(pysha1ctx *self, PyObject *args) {
const char* data;
Py_ssize_t len;
if (!PyArg_ParseTuple(args, PY23("s#", "y#"), &data, &len)) {
return NULL;
}
SHA1DCUpdate(&(self->ctx), data, len);
Py_RETURN_NONE;
}
static PyObject *pysha1ctx_digest(pysha1ctx *self) {
unsigned char hash[20];
if (SHA1DCFinal(hash, &(self->ctx))) {
PyErr_SetString(PyExc_OverflowError, "sha1 collision attack detected");
return NULL;
}
return PyBytes_FromStringAndSize((char *)hash, 20);
}
static PyObject *pysha1ctx_hexdigest(pysha1ctx *self) {
indygregUnsubmitted
Not Done

I'm not super keen on overloading OverflowError here. Or is this how hashlib works in the Python standard library?

indygreg: I'm not super keen on overloading `OverflowError` here. Or is this how `hashlib` works in the…
durin42AuthorUnsubmitted
Done

This is new functionality compared to hashlib (this is the "zomg someone is breaking ur sha1" error case), so there's not anything comparable. I didn't really want to go to the effort of defining our own custom exception type for a case we should never see in the wild.

durin42: This is new functionality compared to hashlib (this is the "zomg someone is breaking ur sha1"…
unsigned char hash[20];
if (SHA1DCFinal(hash, &(self->ctx))) {
PyErr_SetString(PyExc_OverflowError, "sha1 collision attack detected");
return NULL;
}
char hexhash[40];
static const char hexdigit[] = "0123456789abcdef";
for (int i = 0 ; i < 20 ; ++i) {
hexhash[i*2] = hexdigit[hash[i] >> 4];
hexhash[i*2 + 1] = hexdigit[hash[i] & 15];
}
return PyBytes_FromStringAndSize(hexhash, 40);
}
static PyTypeObject sha1ctxType;
static PyObject *pysha1ctx_copy(pysha1ctx *self) {
pysha1ctx *clone = (pysha1ctx *)PyObject_New(pysha1ctx, &sha1ctxType);
if (!clone) {
return NULL;
}
clone->ctx = self->ctx;
return (PyObject *)clone;
}
indygregUnsubmitted
Done

I didn't verify this is correct because I didn't want to think too hard. Test coverage for parity with hashlib would be appreciated.

indygreg: I didn't verify this is correct because I didn't want to think too hard. Test coverage for…
static PyMethodDef pysha1ctx_methods[] = {
{"update", (PyCFunction)pysha1ctx_update, METH_O,
"Update this hash object's state with the provided bytes."},
{"digest", (PyCFunction)pysha1ctx_digest, METH_NOARGS,
"Return the digest value as a string of binary data."},
{"hexdigest",(PyCFunction)pysha1ctx_hexdigest, METH_NOARGS,
"Return the digest value as a string of hexadecimal digits."},
{"copy", (PyCFunction)pysha1ctx_copy, METH_NOARGS,
"Return a copy of the hash object."},
};
spectralUnsubmitted
Done

I think you need a {NULL} entry here, or else it's going to walk off the end of this array when creating the type.

spectral: I think you need a {NULL} entry here, or else it's going to walk off the end of this array when…
durin42AuthorUnsubmitted
Done

Sigh, yep.

durin42: Sigh, yep.
/* clang-format off */
static PyTypeObject sha1ctxType = {
PyVarObject_HEAD_INIT(NULL, 0) /* header */
indygregUnsubmitted
Done

Should this be a memcpy or some such? Or is this opaque type safe to copy by value? Test coverage for this demonstrating that a seeded hasher which is copied can properly diverge would be appreciated.

indygreg: Should this be a `memcpy` or some such? Or is this opaque type safe to copy by value? Test…
durin42AuthorUnsubmitted
Done

I verified by inspection (when writing this) that copying by value is okay here (no pointers, etc), and we now have test coverage that will catch any regression here.

durin42: I verified by inspection (when writing this) that copying by value is okay here (no pointers…
"sha1dc.sha1", /* tp_name */
sizeof(pysha1ctx), /* tp_basicsize */
0, /* tp_itemsize */
(destructor)pysha1ctx_dealloc, /* tp_dealloc */
0, /* tp_print */
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_compare */
0, /* tp_repr */
0, /* tp_as_number */
0, /* tp_as_sequence */
0, /* tp_as_mapping */
0, /* tp_hash */
0, /* tp_call */
0, /* tp_str */
0, /* tp_getattro */
0, /* tp_setattro */
0, /* tp_as_buffer */
Py_TPFLAGS_DEFAULT, /* tp_flags */
"sha1 implementation that looks for collisions", /* tp_doc */
0, /* tp_traverse */
0, /* tp_clear */
0, /* tp_richcompare */
0, /* tp_weaklistoffset */
0, /* tp_iter */
0, /* tp_iternext */
pysha1ctx_methods, /* tp_methods */
0, /* tp_members */
0, /* tp_getset */
0, /* tp_base */
0, /* tp_dict */
0, /* tp_descr_get */
0, /* tp_descr_set */
0, /* tp_dictoffset */
(initproc)pysha1ctx_init, /* tp_init */
0, /* tp_alloc */
};
/* clang-format on */
static PyMethodDef methods[] = {};
spectralUnsubmitted
Done

You may also need a NULL entry here, haven't tracked down where this is used.

spectral: You may also need a NULL entry here, haven't tracked down where this is used.
static void module_init(PyObject *mod)
{
sha1ctxType.tp_new = PyType_GenericNew;
if (PyType_Ready(&sha1ctxType) < 0)
spectralUnsubmitted
Done

why do this here instead of above?

spectral: why do this here instead of above?
durin42AuthorUnsubmitted
Done

Not sure. This is just how I've always seen it done...

durin42: Not sure. This is just how I've always seen it done...
return;
Py_INCREF(&sha1ctxType);
PyModule_AddObject(mod, "sha1", (PyObject *)&sha1ctxType);
}
#ifdef IS_PY3K
static struct PyModuleDef sha1collisiondetection_module = {
PyModuleDef_HEAD_INIT, "sha1collisiondetection", sha1collisiondetection_doc,
-1, methods};
PyMODINIT_FUNC PyInit_sha1dc(void)
{
PyObject *mod = PyModule_Create(&sha1collisiondetection_module);
module_init(mod);
return mod;
}
#else
PyMODINIT_FUNC initsha1dc(void)
{
indygregUnsubmitted
Done

Nit: please always use braces.

indygreg: Nit: please always use braces.
PyObject *mod = Py_InitModule3("sha1collisiondetection", methods,
sha1collisiondetection_doc);
module_init(mod);
}
#endif

setup.py

), ),
Extension( Extension(
'mercurial.thirdparty.zope.interface._zope_interface_coptimizations', 'mercurial.thirdparty.zope.interface._zope_interface_coptimizations',
[ [
'mercurial/thirdparty/zope/interface/_zope_interface_coptimizations.c', 'mercurial/thirdparty/zope/interface/_zope_interface_coptimizations.c',
], ],
), ),
Extension( Extension(
'mercurial.thirdparty.sha1dc',
[
'mercurial/thirdparty/sha1dc/cext.c',
'mercurial/thirdparty/sha1dc/lib/sha1.c',
'mercurial/thirdparty/sha1dc/lib/ubc_check.c',
],
),
Extension(
'hgext.fsmonitor.pywatchman.bser', ['hgext/fsmonitor/pywatchman/bser.c'] 'hgext.fsmonitor.pywatchman.bser', ['hgext/fsmonitor/pywatchman/bser.c']
), ),
RustStandaloneExtension( RustStandaloneExtension(
'mercurial.rustext', 'hg-cpython', 'librusthg', py3_features='python3' 'mercurial.rustext', 'hg-cpython', 'librusthg', py3_features='python3'
), ),
] ]