Skip to content

VIP feedback #1260

@jtsternberg

Description

@jtsternberg

@GaryJones has provided some feedback to get CMB2 in a better state for use on VIP sites. I'm going to list his feedback here.

Hey - here’s the first real batch of CMB2 issues I’ve found (basically from one file). See what you think.

  • includes/CMB2:365 - should be sanitize_html_class, and then wrap in esc_attr().
  • includes/CMB2:511 - needs esc_html() around $label
  • includes/CMB2:514 - needs esc_html() around $description
  • includes/CMB2:529 - needs esc_html() or wp_kses_*() around button content
  • includes/CMB2:574 - potentially incorrect use of sanitize_html_class()
  • includes/CMB2:574 - no need for strip_tags, as sanitize_text_field() does that already.
  • includes/CMB2:596 - id attribute value needs esc_attr()
  • includes/CMB2:596 - data-iterator attribute value needs esc_attr()
  • includes/CMB2:599 - data-selector attribute value needs esc_attr()
  • includes/CMB2:604 - h3 content needs esc_html()
  • includes/CMB2:627 - data-selector attribute value needs esc_attr()
  • includes/CMB2:627 - data-selector attribute value needs esc_attr()
  • includes/CMB2:627 - needs esc_html() or wp_kses_*() around button content
  • includes/CMB2:1011 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1016 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1021 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1033 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1199 - $_GET['page'] input needs sanitization - sanitize_text_field(), etc. though somewhat mitigated by options_page_keys()
  • includes/CMB2:1203 - $_POST['action'] input needs sanitization - sanitize_text_field(), etc. though somewhat mitigated by options_page_keys()
  • includes/CMB2:1206 - The above two items could be mitigated if only a boolean is returned.
  • includes/CMB2:1718 - Nonce isn’t calculated with current time, so could be bypassed with replay attacks > 10 minutes after it is generated. Use wp_create_nonce() instead.

Here’s more:

  • includes/CMB2_Ajax.php:79 - use (int) instead of intval() - casting is about six times quicker.
  • includes/CMB2_Ajax.php:82 - Escaping is typically only done on output - did you mean rawurlencode() or something else?
  • includes/CMB2_Ajax.php:94 - Object ID is not sanitized.
  • includes/CMB2_Ajax.php:95 - Object type is not sanitized.
  • includes/CMB2_Ajax.php:95 - Field ID is not sanitized.
  • includes/CMB2_Ajax.php:181 - oembed needs escaping
  • includes/CMB2_Ajax.php:181 - field ID arg needs escaping
  • includes/CMB2_Base.php:456 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:456 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:456 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Base.php:458 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:458 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:458 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Base.php:462 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:462 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:462 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Base.php:464 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:464 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:464 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Field.php:906 - Attribute values need escaping
  • includes/CMB2_Field.php:916 - Label need escaping with wp_kses_post()
  • includes/CMB2_Field.php:966 - Label attributes need escaping
  • includes/CMB2_Field.php:1119 - Output need escaping
  • includes/CMB2_Field.php:1125 - Attribute values need escaping
  • includes/CMB2_Field.php:1415 - Escaping here is premature. If these are defaults, presumably they can be overridden, and any overridden values should be escaped later anyway.
  • includes/CMB2_Field.php:1416 - Escaping here is premature. If these are defaults, presumably they can be overridden, and any overridden values should be escaped later anyway.
  • includes/CMB2_Field.php:1448 - Escaping here feels premature. Why is the static string escaped, but the dynamic value not? Leave escaping until the context is known.
  • includes/CMB2_Field.php:1452 - Escaping here feels premature. Why is the static string escaped, but the dynamic value not? Leave escaping until the context is known.
  • includes/CMB2_Field_Display.php:132 - sanitize_html_class() and esc_attr() needed here.
  • includes/CMB2_Field_Display.php:135 - List item content needs escaping.
  • includes/CMB2_Field_Display.php:151 - print_r() is considered code for debugging - is there not something better that can be used?
  • includes/CMB2_Field_Display.php:162 - The whole output should be escaped after any URLs have been converted to anchor links - wp_kses_post().
  • includes/CMB2_Field_Display.php:174 - Echo’d content needs escaping.
  • includes/CMB2_Field_Display.php:214 - Fallback needs escaping.
  • includes/CMB2_Field_Display.php:216 - Options value needs escaping.
  • includes/CMB2_Field_Display.php:218 - How do you know that esc_attr() is the correct context to escape with here?
  • includes/CMB2_Field_Display.php:241 - How do you know that esc_attr() is the correct context to escape with here?
  • includes/CMB2_Field_Display.php:245 - Output needs escaping.
  • includes/CMB2_Field_Display.php:245 - Switch wpautop() and wp_kses_post() calls.
  • includes/CMB2_Field_Display.php:267 - <xmp> has been deprecated since HTML 3.2, and removed completely from HTML 5.
  • includes/CMB2_Field_Display.php:278 - Timestamp output needs escaping.
  • includes/CMB2_Field_Display.php:289 - Timestamp output needs escaping.
  • includes/CMB2_Field_Display.php:318 - Timestamp output needs escaping.
  • includes/CMB2_Field_Display.php:379 - Taxonomy should be run through sanitize_html_class().
  • includes/CMB2_Field_Display.php:424 - URL needs escaping with esc_url().
  • includes/CMB2_Field_Display.php:432 - Change esc_html__ to __().
  • includes/CMB2_Field_Display.php:433 - URL needs escaping with esc_url().
  • includes/CMB2_Field_Display.php:434 - File name needs escaping.
  • includes/CMB2_Field_Display.php:457 - Escaping needed.
  • includes/CMB2_Options_Hookup.php:199 - Option key should be run through sanitize_html_class() and esc_attr().
  • includes/CMB2_Options_Hookup.php:199 - ID value should escaped with esc_attr() (or absint() if integer).
  • includes/CMB2_Options_Hookup.php:280 - Nonce check needed, to ensure that the submission came from an authorised source.
  • includes/CMB2_Options_Hookup.php:294 - Possible to use any sort of whitelist approach here, to avoid trying to process malicious data in $_POST?
  • includes/CMB2_Sanitize.php:384 - Why serialize? Serialized data has known vulnerability problems with Object Injection. JSON is generally a better approach for serializing data. See https://www.owasp.org/index.php/PHP_Object_Injection
  • includes/CMB2_Sanitize.php:416 - Would sanitize_textarea_field be better here?
  • includes/CMB2_Types.php:338 - Table ID needs escaping with esc_attr().
  • includes/CMB2_Types.php:344 - Table ID needs escaping with esc_attr().
  • includes/CMB2_Types.php:401 - Classes should be escaped.
  • includes/CMB2_Types.php:435 - Description in span should be escaped.
  • includes/CMB2_Types.php:463 - Unlike _name(), which can be escaped when it is called, the _id() methods adds multiple attribute values, so each should be escaped here.
  • includes/CMB2_hookup.php:461 - ID attribute value should be escaped.
  • includes/CMB2_hookup.php:466 - Span output should be escaped.
  • includes/CMB2_hookup.php:470 - Escaping should be with esc_html(), not esc_attr().
  • includes/CMB2_hookup.php:618 - User ID needs to be sanitized.
  • includes/types/CMB2_Type_Checkbox.php:57 - Double-check that all placeholder replacements are escaped.
  • includes/types/CMB2_Type_File.php:82 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_File.php:106 - src attribute needs to be escaped with esc_url().
  • includes/types/CMB2_Type_File_Base.php:63 - rel attribute needs to be escaped with esc_attr().
  • includes/types/CMB2_Type_File_Base.php:80 - File name string needs to be escaped with esc_html().
  • includes/types/CMB2_Type_File_Base.php:81 - Link value needs to be escaped with esc_url().
  • includes/types/CMB2_Type_File_Base.php:83 - rel attribute needs to be escaped with esc_attr().
  • includes/types/CMB2_Type_File_List.php:41 - Button value should not be escaped with esc_attr() if rendering as <button>, or esc_html__() should be __() if rendering as <input type="button">.
  • includes/types/CMB2_Type_File_List.php:46 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_Multi_Base.php:23 - value attribute should be escaped.
  • includes/types/CMB2_Type_Multi_Base.php:44 - for attribute should be escaped.
  • includes/types/CMB2_Type_Multi_Base.php:44 - Label content should be escaped.
  • includes/types/CMB2_Type_Oembed.php:46 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_Oembed.php:46 - div content should be escaped / wp_kses_post().
  • includes/types/CMB2_Type_Title.php:41 - Span content (name) needs escaping.
  • includes/types/CMB2_Type_Wysiwyg.php:87 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_Wysiwyg.php:89 - Several attribute values should be escaped.
  • js/cmb2.js:58 - HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead.
  • js/cmb2.js:92 - HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead.
  • js/cmb2.js:320 - Any HTML passed to append gets executed. Make sure it’s properly escaped.
  • js/cmb2.js:345 - Any HTML passed to html gets executed. Make sure it’s properly escaped.
  • js/cmb2.js:560 - Any HTML passed to html gets executed. Make sure it’s properly escaped.
  • js/cmb2.js:973 - HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead.
  • js/jquery-ui-timepicker-addon.min.js - include an unminified script for review and debugging.

The final bit are not security issues, but just some observations I made as I was reviewing the code:

  • includes/CMB2_Ajax.php:181 - For accessibility, consider using a button, not an empty anchor.
  • includes/CMB2_Ajax.php:207 - What is this magic number?
  • includes/CMB2_Ajax.php:233 - What is this magic number?
  • includes/CMB2_Field_Display.php:155 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:166 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:178 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:189 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:200 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:223 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:249 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:260 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:271 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:282 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:293 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:322 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:348 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:386 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:441 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:463 - Each class should be in its own file.
  • includes/CMB2_Options.php:44 - Each class should be in its own file.
  • includes/CMB2_Utils.php:298 - Why not return ! CMB_Utils::isempty( $value ); ?
  • includes/CMB2_REST_Controller.php:4 - Is WP_REST_Controller shim still needed?
  • includes/types/CMB2_Type_Textares_Code.php:25 - Why not move the </pre> to line 36 to be more logical?

There were quite a few instances like:

echo '<button type="button" data-selector="', $field_group->id(), '_repeat" data-confirm="', esc_attr( $confirm_deletion ), '" class="dashicons-before dashicons-no-alt cmb-remove-group-row" title="', esc_attr( $field_group->options( 'remove_button' ) ), '"></button>';

That could be made easier to read (and ensure escaping), like:

    '<button type="button" data-selector="%s" data-confirm="%s" class="dashicons-before dashicons-no-alt cmb-remove-group-row" title="%s"></button>',
    esc_attr( $field_group->id() . _repeat ),
    esc_attr( $confirm_deletion ),
    esc_attr( $field_group->options( 'remove_button' ) )
);```

(which some of the later code I was reviewing did do)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions