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

Fix cmp_cli_test issue 13494 #13497

Closed
wants to merge 3 commits into from
Closed

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Nov 24, 2020

  • apps/cmp.c: fix crash with -batch option on OPENSSL_NO_UI_CONSOLE
  • re-encrypt 81-test_cmp_cli_data/Mock/signer.p12 using AES-256-CBC (avoiding DES)

Fixes #13494

@DDvO DDvO mentioned this pull request Nov 24, 2020
@DDvO DDvO requested a review from mattcaswell Nov 24, 2020
NULL, NULL, NULL, NULL, NULL, NULL};
UI_METHOD *ui_fallback_method = (UI_METHOD *)UI_null();
*/
UI_method_set_reader(UI_OpenSSL(), NULL);

This comment has been minimized.

@mattcaswell

mattcaswell Nov 24, 2020
Member

Should we really be "setting" this OpenSSL global method? Shouldn't it be declared as const to prevent this?
I was expecting a solution that copied the UI_METHOD instead. In fact this code looks very much like this existing code where we do make a copy:

openssl/apps/lib/apps_ui.c

Lines 115 to 129 in 9524a30

int setup_ui_method(void)
{
ui_fallback_method = UI_null();
#ifndef OPENSSL_NO_UI_CONSOLE
ui_fallback_method = UI_OpenSSL();
#endif
ui_method = UI_create_method("OpenSSL application user interface");
return ui_method != NULL
&& 0 == UI_method_set_opener(ui_method, ui_open)
&& 0 == UI_method_set_reader(ui_method, ui_read)
&& 0 == UI_method_set_writer(ui_method, ui_write)
&& 0 == UI_method_set_closer(ui_method, ui_close)
&& 0 == UI_method_set_prompt_constructor(ui_method,
ui_prompt_construct);
}

This comment has been minimized.

@mattcaswell

mattcaswell Nov 24, 2020
Member

Shouldn't it be declared as const to prevent this?

Although changing the return value of UI_OpenSSL() itself is probably an API break...so maybe not.

This comment has been minimized.

@DDvO

DDvO Nov 25, 2020
Author Contributor

The code adapting the UI method cmp.c when the -batch option is given had indeed been borrowed from an earlier version of the above quoted code in apps_ui.c.

The goal is to disable any interactive app input, and this so far has been achieved by changing the UI method globally. To admit, this was a pretty invasive way of doing that.
I've just had a closer look and found a more elegant way, namely to modify only the app-specific UI method. This required removing const from the declaration of UI_METHOD *get_ui_method(void), which I believe is adequate.

This comment has been minimized.

@mattcaswell

mattcaswell Nov 25, 2020
Member

I'm still slightly confused....where is the ui method actually used by the cmp app?

This comment has been minimized.

@DDvO

DDvO Nov 25, 2020
Author Contributor

Sigh, it turns out that the more elegant version does not work because load_key_certs_crls() relies on the UI method returned by get_ui_method(), so changed the fix back to adapting UI_OpenSSL() instead.

This comment has been minimized.

@DDvO

DDvO Nov 25, 2020
Author Contributor

where is the ui method actually used by the cmp app?

The UI methods are used by all apps using load_key() and the like, which rely on load_key_certs_crls().

This comment has been minimized.

@t8m

t8m Nov 25, 2020
Member

I suppose the clean fix would be to to add set_batch_ui_method() which would set the NULL reader. in the ui_method returned by get_ui_method?

This comment has been minimized.

@DDvO

DDvO Nov 25, 2020
Author Contributor

As I tried to explain above removing the reader method from the apps' ui_method (regardless which way this is done) is not the right solution because load_key_certs_crls() unfortunately uses this UI method for passing further down the pass argument. So any given password is lost this way :-(

This comment has been minimized.

@t8m

t8m Nov 25, 2020
Member

Ah, now I get it. But you could then set a flag to just return null and not call the reader from ui_fallback_method in case the password is not set.

This comment has been minimized.

@DDvO

DDvO Nov 25, 2020
Author Contributor

The mentioned problem is in case a password must be used, not when it is not set).

I suggest doing any further discussion on this later
because getting this fix merged is urgent to fix CI test failures
and I have other very urgent things to do today.

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Nov 24, 2020

I think this fixes the minimal CI test that's failing for other pull requests, so I put an urgent label on it.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Nov 25, 2020

The sanitizers CI failure is unrelated.

@DDvO DDvO force-pushed the siemens:fix_cmp_cli_test_13494 branch Nov 25, 2020
@kroeckx
Copy link
Member

@kroeckx kroeckx commented Nov 25, 2020

@DDvO

This comment has been hidden.

@DDvO DDvO requested a review from mattcaswell Nov 25, 2020
@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 25, 2020

Hmm CI failures still...

@DDvO DDvO force-pushed the siemens:fix_cmp_cli_test_13494 branch Nov 25, 2020
@DDvO DDvO force-pushed the siemens:fix_cmp_cli_test_13494 branch to 6758bfe Nov 25, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Nov 25, 2020

Hmm CI failures still...

This should be fixed now; see my above comment on your review.

BTW, rebasing on the current master lead to an unrelated build failure on test/pem_read_depr_test
So I've provisionally rebased this PR on the slightly earlier commit ee82528.

@@ -136,7 +136,7 @@ void destroy_ui_method(void)
}
}

const UI_METHOD *get_ui_method(void)
UI_METHOD *get_ui_method(void)

This comment has been minimized.

@mattcaswell

mattcaswell Nov 25, 2020
Member

Should we revert this change too?

This comment has been minimized.

@DDvO

DDvO Nov 25, 2020
Author Contributor

I suggest leaving this in because it may be useful in other situations.

Copy link
Member

@mattcaswell mattcaswell left a comment

I'm not too enthusiastic about the UI_METHOD stuff. But my concern there pre-dates this PR, and this PR improves the CI situation. So approved.

openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
…iding DES)

Fixes #13494

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #13497)
openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #13497)
openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
Also make clear we cannot use get_ui_method() at this point.

Fixes #13494

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #13497)
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Nov 25, 2020

I'm not too enthusiastic about the UI_METHOD stuff. But my concern there pre-dates this PR, and this PR improves the CI situation. So approved.

Merged already now since this was urgent.

Shall I keep this PR open for now as a reminder to finish the above discussion?

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 25, 2020

Shall I keep this PR open for now as a reminder to finish the above discussion?

I raised issue #13511 for this, so I think this PR can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants