Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update md5.h / md5.c from upstream OpenBSD #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DimitriPapadopoulos
Copy link
Contributor

Changes are minimal, as upstream itself changed bzero() → memset().

Note that the new bzero_explicit() function call in upstream has been changed to a memset() function call, in the absence of a widely available memset_explicit() function for now.

Also update the URL of the OpenBSD source repository.

PUT_32BIT_LE(digest + i * 4, ctx->state[i]);
}
for (i = 0; i < 4; i++)
PUT_32BIT_LE(digest + i * 4, ctx->state[i]);
memset(ctx, 0, sizeof(*ctx)); /* in case it's sensitive */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where upstream introduced a bzero_explicit() call, which we keep translating into a memset() call.

@alandekok
Copy link
Member

Are there any functionality changes here? I'm not sure why these changes are needed.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jul 31, 2024

No functionality changes. The rationale is that upstream have changed bzero() to memset() in their latest version 1.4. The header claims that the following changes have been applied to upstream version 1.1:

 *	with the following changes:
 *	#includes commented out.
 *	Support context->count as uint32_t[2] instead of uint64_t
 *	u_int* to uint*

That would be true compared to the latest upstream version 1.4, but not compared to upstream version 1.1, where the following comment would be required:

 *	change all occurrences of bzero() to memset()

I find it better to update the base upstream version from 1.1 to the latest 1.4, since it includes changes that have also been applied here, than documenting these changes in the header.

Here is the diff between upstream 1.1 and 1.4:
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/crypto/md5.c.diff?r1=1.1&r2=1.4&f=h

This PR also fixes the URL of the OpenBSD source repository.

Finally, I wanted to point out that the explicit_bzero() call from upstream 1.4 has been transformed into a mere memset(). Perhaps I should add this to the changes listed in the comments, shouldn't I?

@DimitriPapadopoulos
Copy link
Contributor Author

It just occurred to me that I should also update md5.h from 1.1 to 1.3, for consistency:

https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/crypto/md5.h.diff?r1=1.1&r2=1.3&f=h

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the md5.c_1.4 branch 3 times, most recently from 021da95 to 8bf6b33 Compare July 31, 2024 13:50
@DimitriPapadopoulos DimitriPapadopoulos changed the title Update md5.c from upstream OpenBSD: 1.1 → 1.4 Update md5.c from upstream OpenBSD Jul 31, 2024
md5.h: 1.1 → 1.3
md5.c: 1.1 → 1.4

Changes are minimal, as upstream itself changed bzero() → memset().

Note that the new bzero_explicit() function call in upstream has been
changed to a memset() function call, in the absence of a widely available
memset_explicit() function for now.

Also update the URL of the OpenBSD source repository.
@DimitriPapadopoulos DimitriPapadopoulos changed the title Update md5.c from upstream OpenBSD Update md5.h / md5.c from upstream OpenBSD Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants