diff --git a/crypto/x509/x509_att.c b/crypto/x509/x509_att.c
index 90f722a..3773829 100644
--- a/crypto/x509/x509_att.c
+++ b/crypto/x509/x509_att.c
@@ -218,7 +218,7 @@
 }
 
 X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_NID(X509_ATTRIBUTE **attr, int nid,
-                                             int atrtype, const void *data,
+                                             int attrtype, const void *data,
                                              int len)
 {
     const ASN1_OBJECT *obj;
@@ -228,12 +228,12 @@
         OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_NID);
         return (NULL);
     }
-    return X509_ATTRIBUTE_create_by_OBJ(attr, obj, atrtype, data, len);
+    return X509_ATTRIBUTE_create_by_OBJ(attr, obj, attrtype, data, len);
 }
 
 X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_OBJ(X509_ATTRIBUTE **attr,
                                              const ASN1_OBJECT *obj,
-                                             int atrtype, const void *data,
+                                             int attrtype, const void *data,
                                              int len)
 {
     X509_ATTRIBUTE *ret;
@@ -248,7 +248,7 @@
 
     if (!X509_ATTRIBUTE_set1_object(ret, obj))
         goto err;
-    if (!X509_ATTRIBUTE_set1_data(ret, atrtype, data, len))
+    if (!X509_ATTRIBUTE_set1_data(ret, attrtype, data, len))
         goto err;
 
     if ((attr != NULL) && (*attr == NULL))
@@ -261,17 +261,17 @@
 }
 
 X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_txt(X509_ATTRIBUTE **attr,
-                                             const char *atrname, int type,
+                                             const char *attrname, int type,
                                              const unsigned char *bytes,
                                              int len)
 {
     ASN1_OBJECT *obj;
     X509_ATTRIBUTE *nattr;
 
-    obj = OBJ_txt2obj(atrname, 0);
+    obj = OBJ_txt2obj(attrname, 0);
     if (obj == NULL) {
         OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_NAME);
-        ERR_add_error_data(2, "name=", atrname);
+        ERR_add_error_data(2, "name=", attrname);
         return (NULL);
     }
     nattr = X509_ATTRIBUTE_create_by_OBJ(attr, obj, type, bytes, len);
@@ -339,7 +339,7 @@
     return 0;
 }
 
-int X509_ATTRIBUTE_count(X509_ATTRIBUTE *attr)
+int X509_ATTRIBUTE_count(const X509_ATTRIBUTE *attr)
 {
     return sk_ASN1_TYPE_num(attr->set);
 }
@@ -352,13 +352,13 @@
 }
 
 void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx,
-                               int atrtype, void *unused)
+                               int attrtype, void *unused)
 {
     ASN1_TYPE *ttmp;
     ttmp = X509_ATTRIBUTE_get0_type(attr, idx);
     if (!ttmp)
         return NULL;
-    if (atrtype != ASN1_TYPE_get(ttmp)) {
+    if (attrtype != ASN1_TYPE_get(ttmp)) {
         OPENSSL_PUT_ERROR(X509, X509_R_WRONG_TYPE);
         return NULL;
     }
diff --git a/crypto/x509/x509_req.c b/crypto/x509/x509_req.c
index d15c05f..202ae3f 100644
--- a/crypto/x509/x509_req.c
+++ b/crypto/x509/x509_req.c
@@ -251,31 +251,31 @@
 }
 
 int X509_REQ_add1_attr_by_OBJ(X509_REQ *req,
-                              const ASN1_OBJECT *obj, int type,
-                              const unsigned char *bytes, int len)
+                              const ASN1_OBJECT *obj, int attrtype,
+                              const unsigned char *data, int len)
 {
     if (X509at_add1_attr_by_OBJ(&req->req_info->attributes, obj,
-                                type, bytes, len))
+                                attrtype, data, len))
         return 1;
     return 0;
 }
 
 int X509_REQ_add1_attr_by_NID(X509_REQ *req,
-                              int nid, int type,
-                              const unsigned char *bytes, int len)
+                              int nid, int attrtype,
+                              const unsigned char *data, int len)
 {
     if (X509at_add1_attr_by_NID(&req->req_info->attributes, nid,
-                                type, bytes, len))
+                                attrtype, data, len))
         return 1;
     return 0;
 }
 
 int X509_REQ_add1_attr_by_txt(X509_REQ *req,
-                              const char *attrname, int type,
-                              const unsigned char *bytes, int len)
+                              const char *attrname, int attrtype,
+                              const unsigned char *data, int len)
 {
     if (X509at_add1_attr_by_txt(&req->req_info->attributes, attrname,
-                                type, bytes, len))
+                                attrtype, data, len))
         return 1;
     return 0;
 }
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 57641bb..048e9ce 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -33,6 +33,9 @@
 #include "../internal.h"
 #include "../test/test_util.h"
 #include "../x509v3/internal.h"
+#include "openssl/asn1.h"
+#include "openssl/base.h"
+#include "openssl/nid.h"
 
 
 std::string GetTestData(const char *path);
@@ -3120,3 +3123,85 @@
     }
   }
 }
+
+// Test the various |X509_ATTRIBUTE| creation functions.
+TEST(X509Test, Attribute) {
+  // The friendlyName attribute has a BMPString value. See RFC2985,
+  // section 5.5.1.
+  static const uint8_t kTest1[] = {0x26, 0x03};  // U+2603 SNOWMAN
+  static const uint8_t kTest1UTF8[] = {0xe2, 0x98, 0x83};
+  static const uint8_t kTest2[] = {0, 't', 0, 'e', 0, 's', 0, 't'};
+
+  auto check_attribute = [&](X509_ATTRIBUTE *attr, bool has_test2) {
+    EXPECT_EQ(NID_friendlyName, OBJ_obj2nid(X509_ATTRIBUTE_get0_object(attr)));
+
+    EXPECT_EQ(has_test2 ? 2 : 1, X509_ATTRIBUTE_count(attr));
+
+    // The first attribute should contain |kTest1|.
+    const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, 0);
+    ASSERT_TRUE(value);
+    EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
+    EXPECT_EQ(Bytes(kTest1),
+              Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
+                    ASN1_STRING_length(value->value.bmpstring)));
+
+    // |X509_ATTRIBUTE_get0_data| requires the type match.
+    EXPECT_FALSE(
+        X509_ATTRIBUTE_get0_data(attr, 0, V_ASN1_OCTET_STRING, nullptr));
+    const ASN1_BMPSTRING *bmpstring = static_cast<const ASN1_BMPSTRING *>(
+        X509_ATTRIBUTE_get0_data(attr, 0, V_ASN1_BMPSTRING, nullptr));
+    ASSERT_TRUE(bmpstring);
+    EXPECT_EQ(Bytes(kTest1), Bytes(ASN1_STRING_get0_data(bmpstring),
+                                   ASN1_STRING_length(bmpstring)));
+
+    if (has_test2) {
+      value = X509_ATTRIBUTE_get0_type(attr, 1);
+      ASSERT_TRUE(value);
+      EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
+      EXPECT_EQ(Bytes(kTest2),
+                Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
+                      ASN1_STRING_length(value->value.bmpstring)));
+    } else {
+      EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 1));
+    }
+
+    EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 2));
+  };
+
+  bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_type_new(V_ASN1_BMPSTRING));
+  ASSERT_TRUE(str);
+  ASSERT_TRUE(ASN1_STRING_set(str.get(), kTest1, sizeof(kTest1)));
+
+  // Test |X509_ATTRIBUTE_create|.
+  bssl::UniquePtr<X509_ATTRIBUTE> attr(
+      X509_ATTRIBUTE_create(NID_friendlyName, V_ASN1_BMPSTRING, str.get()));
+  ASSERT_TRUE(attr);
+  str.release();  // |X509_ATTRIBUTE_create| takes ownership on success.
+  check_attribute(attr.get(), /*has_test2=*/false);
+
+  // Test the |MBSTRING_*| form of |X509_ATTRIBUTE_set1_data|.
+  attr.reset(X509_ATTRIBUTE_new());
+  ASSERT_TRUE(attr);
+  ASSERT_TRUE(
+      X509_ATTRIBUTE_set1_object(attr.get(), OBJ_nid2obj(NID_friendlyName)));
+  ASSERT_TRUE(X509_ATTRIBUTE_set1_data(attr.get(), MBSTRING_UTF8, kTest1UTF8,
+                                       sizeof(kTest1UTF8)));
+  check_attribute(attr.get(), /*has_test2=*/false);
+
+  // Test the |ASN1_STRING| form of |X509_ATTRIBUTE_set1_data|.
+  ASSERT_TRUE(X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_BMPSTRING, kTest2,
+                                       sizeof(kTest2)));
+  check_attribute(attr.get(), /*has_test2=*/true);
+
+  // Test the |ASN1_TYPE| form of |X509_ATTRIBUTE_set1_data|.
+  attr.reset(X509_ATTRIBUTE_new());
+  ASSERT_TRUE(attr);
+  ASSERT_TRUE(
+      X509_ATTRIBUTE_set1_object(attr.get(), OBJ_nid2obj(NID_friendlyName)));
+  str.reset(ASN1_STRING_type_new(V_ASN1_BMPSTRING));
+  ASSERT_TRUE(str);
+  ASSERT_TRUE(ASN1_STRING_set(str.get(), kTest1, sizeof(kTest1)));
+  ASSERT_TRUE(
+      X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_BMPSTRING, str.get(), -1));
+  check_attribute(attr.get(), /*has_test2=*/false);
+}
diff --git a/crypto/x509/x_attrib.c b/crypto/x509/x_attrib.c
index 3ee8bd4..91b3ee8 100644
--- a/crypto/x509/x_attrib.c
+++ b/crypto/x509/x_attrib.c
@@ -70,7 +70,7 @@
 IMPLEMENT_ASN1_FUNCTIONS(X509_ATTRIBUTE)
 IMPLEMENT_ASN1_DUP_FUNCTION(X509_ATTRIBUTE)
 
-X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int atrtype, void *value)
+X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int attrtype, void *value)
 {
     ASN1_OBJECT *obj = OBJ_nid2obj(nid);
     if (obj == NULL) {
@@ -88,7 +88,7 @@
         goto err;
     }
 
-    ASN1_TYPE_set(val, atrtype, value);
+    ASN1_TYPE_set(val, attrtype, value);
     return ret;
 
  err:
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 168cac4..3eb7501 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -980,7 +980,12 @@
 DECLARE_ASN1_FUNCTIONS(X509_REQ)
 
 DECLARE_ASN1_FUNCTIONS(X509_ATTRIBUTE)
-OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int atrtype,
+
+// X509_ATTRIBUTE_create returns a newly-allocated |X509_ATTRIBUTE|, or NULL on
+// error. The attribute has type |nid| and contains a single value determined by
+// |attrtype| and |value|, which are interpreted as in |ASN1_TYPE_set|. Note
+// this function takes ownership of |value|.
+OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create(int nid, int attrtype,
                                                      void *value);
 
 DECLARE_ASN1_FUNCTIONS(X509_EXTENSION)
@@ -1283,16 +1288,30 @@
 // TODO(https://crbug.com/boringssl/407): |attr| should be const.
 OPENSSL_EXPORT int X509_REQ_add1_attr(X509_REQ *req, X509_ATTRIBUTE *attr);
 
+// X509_REQ_add1_attr_by_OBJ appends a new attribute to |req| with type |obj|.
+// It returns one on success and zero on error. The value is determined by
+// |X509_ATTRIBUTE_set1_data|.
+//
+// WARNING: The interpretation of |attrtype|, |data|, and |len| is complex and
+// error-prone. See |X509_ATTRIBUTE_set1_data| for details.
 OPENSSL_EXPORT int X509_REQ_add1_attr_by_OBJ(X509_REQ *req,
-                                             const ASN1_OBJECT *obj, int type,
-                                             const unsigned char *bytes,
+                                             const ASN1_OBJECT *obj,
+                                             int attrtype,
+                                             const unsigned char *data,
                                              int len);
-OPENSSL_EXPORT int X509_REQ_add1_attr_by_NID(X509_REQ *req, int nid, int type,
-                                             const unsigned char *bytes,
+
+// X509_REQ_add1_attr_by_NID behaves like |X509_REQ_add1_attr_by_OBJ| except the
+// attribute type is determined by |nid|.
+OPENSSL_EXPORT int X509_REQ_add1_attr_by_NID(X509_REQ *req, int nid,
+                                             int attrtype,
+                                             const unsigned char *data,
                                              int len);
+
+// X509_REQ_add1_attr_by_txt behaves like |X509_REQ_add1_attr_by_OBJ| except the
+// attribute type is determined by calling |OBJ_txt2obj| with |attrname|.
 OPENSSL_EXPORT int X509_REQ_add1_attr_by_txt(X509_REQ *req,
-                                             const char *attrname, int type,
-                                             const unsigned char *bytes,
+                                             const char *attrname, int attrtype,
+                                             const unsigned char *data,
                                              int len);
 
 // X509_CRL_set_version sets |crl|'s version to |version|, which should be one
@@ -1632,22 +1651,96 @@
 OPENSSL_EXPORT void *X509at_get0_data_by_OBJ(STACK_OF(X509_ATTRIBUTE) *x,
                                              ASN1_OBJECT *obj, int lastpos,
                                              int type);
+
+// X509_ATTRIBUTE_create_by_NID returns a newly-allocated |X509_ATTRIBUTE| of
+// type |nid|, or NULL on error. The value is determined as in
+// |X509_ATTRIBUTE_set1_data|.
+//
+// If |attr| is non-NULL, the resulting |X509_ATTRIBUTE| is also written to
+// |*attr|. If |*attr| was non-NULL when the function was called, |*attr| is
+// reused instead of creating a new object.
+//
+// WARNING: The interpretation of |attrtype|, |data|, and |len| is complex and
+// error-prone. See |X509_ATTRIBUTE_set1_data| for details.
+//
+// WARNING: The object reuse form is deprecated and may be removed in the
+// future. It also currently incorrectly appends to the reused object's value
+// set rather than overwriting it.
 OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_NID(
-    X509_ATTRIBUTE **attr, int nid, int atrtype, const void *data, int len);
+    X509_ATTRIBUTE **attr, int nid, int attrtype, const void *data, int len);
+
+// X509_ATTRIBUTE_create_by_OBJ behaves like |X509_ATTRIBUTE_create_by_NID|
+// except the attribute's type is determined by |obj|.
 OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_OBJ(
-    X509_ATTRIBUTE **attr, const ASN1_OBJECT *obj, int atrtype,
+    X509_ATTRIBUTE **attr, const ASN1_OBJECT *obj, int attrtype,
     const void *data, int len);
+
+// X509_ATTRIBUTE_create_by_txt behaves like |X509_ATTRIBUTE_create_by_NID|
+// except the attribute's type is determined by calling |OBJ_txt2obj| with
+// |attrname|.
 OPENSSL_EXPORT X509_ATTRIBUTE *X509_ATTRIBUTE_create_by_txt(
-    X509_ATTRIBUTE **attr, const char *atrname, int type,
+    X509_ATTRIBUTE **attr, const char *attrname, int type,
     const unsigned char *bytes, int len);
+
+// X509_ATTRIBUTE_set1_object sets |attr|'s type to |obj|. It returns one on
+// success and zero on error.
 OPENSSL_EXPORT int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr,
                                               const ASN1_OBJECT *obj);
+
+// X509_ATTRIBUTE_set1_data appends a value to |attr|'s value set and returns
+// one on success or zero on error. The value is determined as follows:
+//
+// If |attrtype| is a |MBSTRING_*| constant, the value is an ASN.1 string. The
+// string is determined by decoding |len| bytes from |data| in the encoding
+// specified by |attrtype|, and then re-encoding it in a form appropriate for
+// |attr|'s type. If |len| is -1, |strlen(data)| is used instead. See
+// |ASN1_STRING_set_by_NID| for details.
+//
+// TODO(davidben): Document |ASN1_STRING_set_by_NID| so the reference is useful.
+//
+// Otherwise, if |len| is not -1, the value is an ASN.1 string. |attrtype| is an
+// |ASN1_STRING| type value and the |len| bytes from |data| are copied as the
+// type-specific representation of |ASN1_STRING|. See |ASN1_STRING| for details.
+//
+// WARNING: If this form is used to construct a negative INTEGER or ENUMERATED,
+// |attrtype| includes the |V_ASN1_NEG| flag for |ASN1_STRING|, but the function
+// forgets to clear the flag for |ASN1_TYPE|. This matches OpenSSL but is
+// probably a bug. For now, do not use this form with negative values.
+//
+// Otherwise, if |len| is -1, the value is constructed by passing |attrtype| and
+// |data| to |ASN1_TYPE_set1|. That is, |attrtype| is an |ASN1_TYPE| type value,
+// and |data| is cast to the corresponding pointer type.
+//
+// WARNING: Despite the name, this function appends to |attr|'s value set,
+// rather than overwriting it. To overwrite the value set, create a new
+// |X509_ATTRIBUTE| with |X509_ATTRIBUTE_new|.
+//
+// WARNING: If using the |MBSTRING_*| form, pass a length rather than relying on
+// |strlen|. In particular, |strlen| will not behave correctly if the input is
+// |MBSTRING_BMP| or |MBSTRING_UNIV|.
+//
+// WARNING: This function currently misinterprets |V_ASN1_OTHER| as an
+// |MBSTRING_*| constant. This matches OpenSSL but means it is impossible to
+// construct a value with a non-universal tag.
 OPENSSL_EXPORT int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype,
                                             const void *data, int len);
+
+// X509_ATTRIBUTE_get0_data returns the |idx|th value of |attr| in a
+// type-specific representation to |attrtype|, or NULL if out of bounds or the
+// type does not match. |attrtype| is one of the type values in |ASN1_TYPE|. On
+// match, the return value uses the same representation as |ASN1_TYPE_set0|. See
+// |ASN1_TYPE| for details.
 OPENSSL_EXPORT void *X509_ATTRIBUTE_get0_data(X509_ATTRIBUTE *attr, int idx,
-                                              int atrtype, void *unused);
-OPENSSL_EXPORT int X509_ATTRIBUTE_count(X509_ATTRIBUTE *attr);
+                                              int attrtype, void *unused);
+
+// X509_ATTRIBUTE_count returns the number of values in |attr|.
+OPENSSL_EXPORT int X509_ATTRIBUTE_count(const X509_ATTRIBUTE *attr);
+
+// X509_ATTRIBUTE_get0_object returns the type of |attr|.
 OPENSSL_EXPORT ASN1_OBJECT *X509_ATTRIBUTE_get0_object(X509_ATTRIBUTE *attr);
+
+// X509_ATTRIBUTE_get0_type returns the |idx|th value in |attr|, or NULL if out
+// of bounds. Note this function returns one of |attr|'s values, not the type.
 OPENSSL_EXPORT ASN1_TYPE *X509_ATTRIBUTE_get0_type(X509_ATTRIBUTE *attr,
                                                    int idx);
 
@@ -1742,6 +1835,7 @@
 BORINGSSL_MAKE_DELETER(X509, X509_free)
 BORINGSSL_MAKE_UP_REF(X509, X509_up_ref)
 BORINGSSL_MAKE_DELETER(X509_ALGOR, X509_ALGOR_free)
+BORINGSSL_MAKE_DELETER(X509_ATTRIBUTE, X509_ATTRIBUTE_free)
 BORINGSSL_MAKE_DELETER(X509_CRL, X509_CRL_free)
 BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref)
 BORINGSSL_MAKE_DELETER(X509_CRL_METHOD, X509_CRL_METHOD_free)