DERGeneralEncoder is signature-compatible with a copy constructor when its default arguments are taken into account.

11 views

Skip to first unread message

Jeffrey Walton

unread,

Jun 3, 2019, 6:05:20 AM6/3/19

to Crypto++ Users

Hi Everyone,

We started running Crypto++ through https://lgtm.com, which provides security related recommendations. We're seeing some old warnings like: DERGeneralEncoder is signature-compatible with a copy constructor when its default arguments are taken into account:

DERGeneralEncoder::DERGeneralEncoder(DERGeneralEncoder &outQueue, byte asnTag)
: ByteQueue(), m_outQueue(outQueue), m_asnTag(asnTag), m_finished(false)
{
}

I think that was a design choice by Wei early in the library. I think the complaint is mostly style, but I don't like that it is getting in the way of a quick-and-dirty security evaluation. That's a recipe for bug reports, mailing list messages and failed audits.

My question is, should we clear them?

The potential side effect is, if we clear them, then we break ABI and have to major bump the library.

Jeff

Jeffrey Walton

unread,

Jun 4, 2019, 3:08:28 AM6/4/19

to Crypto++ Users

On Monday, June 3, 2019 at 6:05:20 AM UTC-4, Jeffrey Walton wrote:

Hi Everyone,

We started running Crypto++ through https://lgtm.com, which provides security related recommendations. We're seeing some old warnings like: DERGeneralEncoder is signature-compatible with a copy constructor when its default arguments are taken into account...

I think that was a design choice by Wei early in the library. I think the complaint is mostly style, but I don't like that it is getting in the way of a quick-and-dirty security evaluation. That's a recipe for bug reports, mailing list messages and failed audits.

My question is, should we clear them?

We moved forward with clearing the findings so they would not creep into someone's security evaluation of the library. Also see https://github.com/weidai11/cryptopp/commit/a6440086792 .

We ran cryptest-symbols.sh on the change, and it does not appear symbols went missing. I think we should be OK to stay the course with a minor version bump.