Hi, I worked on my pkcs11 extension some more, and I believe it is ready for at least a 0.1 release. I have addressed the memory management issues and implemented the remaining important features.
According to the PECL registration form, I need to obtain determination that "it's time for me to have a PECL account". Does that sound like the next step ? Also, feel free to provide more commentary on my work if you think it is pertinent. *Guillaume Amringerhttps://keybase.io/gamr <https://keybase.io/gamr>https://www.linkedin.com/in/guillaumeamringer <https://www.linkedin.com/in/guillaumeamringer>https://github.com/gamringer <https://github.com/gamringer>* On Tue, Jul 21, 2020 at 10:38 PM Guillaume Amringer <g.amrin...@gmail.com> wrote: > Hi all, > > Thanks for the feedback. I was mostly concerned with the happy path at > first, fixing memory management was on my list, but given it's the > recurring comment, I am starting on that now. This is my first real C > project, so your criticism is welcome. > > Regarding the API, I can't simply expose the same API as the definition, > since at least, there won't be the need to pass the various argument > lengths in PHP land. Since I was going to change the API anyway, I thought > it best to use the recommended code convention for class and method names. > It is also in line with other languages PKCS11 implementations. One > compromise I think is appropriate would be to provide the vanilla API as > functions (as they are) taking a Pkcs11\Session object as the session > handler. and leave it up to the user to choose how to use it. @vjardin, > what do you think ? > > Regarding C_GetAttributeValue() return code, this is more of a design > choice, since I absolutely despise passing a variable reference to be > filled by the method, returning a partial result, along with an explanation > would require some hoop jumping. Not impossible, but worth thinking it > over. What I see is returning a new AttributeSet object, with isComplete(): > bool (true if CKR_OK, false otherwise), and getError() returning the code. > Not married to it though, I am open to suggestions. > > https://python-pkcs11.readthedocs.io/en/latest/applied.html > https://godoc.org/github.com/miekg/pkcs11 > > Thank you for taking the time to comment. > > > > > > *Guillaume Amringerhttps://keybase.io/gamr > <https://keybase.io/gamr>https://www.linkedin.com/in/guillaumeamringer > <https://www.linkedin.com/in/guillaumeamringer>https://github.com/gamringer > <https://github.com/gamringer>* > > > On Mon, Jul 20, 2020 at 3:46 PM <vjar...@free.fr> wrote: > >> Hi Guillaume, >> >> same comments than Johannes, ecalloc()/efree()/emalloc() should be used. >> Since they are many crypto sensitive contents, only ecalloc() should be >> used (no emalloc()). >> >> Regarding the Oasis' semantic, I feel that it is important to keep it "as >> is" so the vanilla documentation would be the upstream Oasis PKCS11 >> standard. >> It means, for instance: >> getInfo() -> C_GetInfo() >> >> It is nice to have some sign() methods in order to avoid combining >> C_SignInit() + C_Sign(), but they are some cases when we need to stick with >> the Oasis' specifications, so I believe that >> C_SignInit() >> C_VerifyInit() >> C_FindObjectsInit() >> C_FindObjectsFinal() >> etc. >> should still be exposed per the Oasis' specification, along having some >> nice sign(), verify(), etc. methods too. >> >> Regarding: >> C_GetAttributeValue() >> it has many "OK" cases: >> See Oasis' comments: >> "Note that the error codes CKR_ATTRIBUTE_SENSITIVE, >> CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote true >> errors for C_GetAttributeValue. If a call to C_GetAttributeValue returns >> any of these three values, then the call MUST nonetheless have processed >> every attribute in the template supplied to C_GetAttributeValue. Each >> attribute in the template whose value can be returned by the call to >> C_GetAttributeValue will be returned by the call to C_GetAttributeValue." >> >> my 2 cents, >> Vincent >> >> ----- Mail original ----- >> De: "Johannes Schlüter" <johan...@schlueters.de> >> À: "Guillaume Amringer" <g.amrin...@gmail.com>, "pecl-dev" < >> pecl-dev@lists.php.net> >> Envoyé: Lundi 20 Juillet 2020 14:55:33 >> Objet: Re: [PECL-DEV] PKCS11 Extension >> >> Hi, >> >> On Sun, 2020-07-19 at 22:33 -0400, Guillaume Amringer wrote: >> > The extension is currently hosted here: >> > https://github.com/gamringer/php-pkcs11 >> >> I scrolled over it without much attention (thus probably missed other >> issues) a few comments to this line I noticed: >> >> pSlotList = (CK_SLOT_ID_PTR) malloc(ulSlotCount * sizeof(CK_SLOT_ID)); >> https://github.com/gamringer/php-pkcs11/blob/master/pkcs11.c#L300 >> >> 1. The return value of malloc() is not checked. If the system runs out >> of memory (OOM) or for some other reason can't return a memory block >> this will return NULL and then lead to undefined behavior later. >> (unlikely on today's systems, but might happen) >> 2. emalloc should be used instead of malloc. For one it counts towards >> PHP's memory_limit, ten it also ensures the memory is freed if >> something weird happens by the end of the request, thus reduces risk >> of memory leaks and it fixes point 1. by terminating the request in >> an OOM situation. >> 3. ulSlotCount * sizeof(CK_SLOT_ID) could eventually overflow. Better >> use >> safe_emalloc(ulSlotCount, sizeof(CK_SLOT_ID), 0) >> this >> calculates >> ulSlotCount * sizeof(CK_SLOT_ID) + 0 >> in a overflow-safe >> way and errors out in case of a problem. >> >> johannes >> >> -- >> PECL development discussion Mailing List (https://pecl.php.net/) >> To unsubscribe, visit: https://www.php.net/unsub.php >> >
