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

CRL: decode stored crl data before display #5965

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

kulikov-a
Copy link
Member

@kulikov-a kulikov-a commented Aug 20, 2022

Hi!
ref. https://forum.opnsense.org/index.php?topic=29937.0
Turned out that administrator may accidentally break the imported CRL by clicking Save when viewing crl data: CRL data field shows encoded string.
Thanks!

decode existing crl text
@kulikov-a kulikov-a changed the title CRL: decode existing crl before show CRL: decode stored crl data before display Aug 21, 2022
@fichtner
Copy link
Member

This is all a bit strange... if we save base64 encoded data and expect users to paste base64 encoded data (they certainly cannot paste binary data into a textarea very well) where is the need for any sort of decode/encode internally?

@kulikov-a
Copy link
Member Author

@fichtner Hi! hm..if we encode user input for imorted/existing crl at

if (($pconfig['crlmethod'] == "existing") || ($act == "editimported")) {
$crl['text'] = base64_encode($pconfig['crltext']);
}

i think we need to decode it before display (or we will double encode it later on save? ;)

@swhite2
Copy link
Member

swhite2 commented Aug 22, 2022

This is all a bit strange... if we save base64 encoded data and expect users to paste base64 encoded data (they certainly cannot paste binary data into a textarea very well) where is the need for any sort of decode/encode internally?

Do we expect base64 encoded data though?

@kulikov-a
Copy link
Member Author

since all other certs data is encoded in conf (keys, internal crl's etc) i found it quiet logical )

@fichtner
Copy link
Member

I don't think we double-encode all PEM related input. It is what it is (and there could be more bugs).

@kulikov-a
Copy link
Member Author

kulikov-a commented Aug 22, 2022

yep. and, for example, on the system_camanager.php CA edit page, config certificates are pre-decoded:

if ($act == "edit") {
if (!isset($id)) {
header(url_safe('Location: /system_camanager.php'));
exit;
}
$pconfig['descr'] = $a_ca[$id]['descr'];
$pconfig['refid'] = $a_ca[$id]['refid'];
$pconfig['cert'] = base64_decode($a_ca[$id]['crt']);
$pconfig['serial'] = $a_ca[$id]['serial'] + 1;
if (!empty($a_ca[$id]['prv'])) {
$pconfig['key'] = base64_decode($a_ca[$id]['prv']);
}

Or did I miss the thread somewhere?

@swhite2 swhite2 merged commit 88011ed into opnsense:master Aug 23, 2022
@swhite2
Copy link
Member

swhite2 commented Aug 23, 2022

@kulikov-a Thanks!

@kulikov-a
Copy link
Member Author

Thanks!)

@kulikov-a kulikov-a deleted the patch-17 branch August 23, 2022 09:19
fichtner pushed a commit that referenced this pull request Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants